Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CALCITE-5570] Support nested map type for SqlDataTypeSpec #3105

Merged
merged 1 commit into from
Oct 1, 2023
Merged

[CALCITE-5570] Support nested map type for SqlDataTypeSpec #3105

merged 1 commit into from
Oct 1, 2023

Conversation

JiajunBernoulli
Copy link
Contributor

No description provided.

snuyanzin
snuyanzin reviewed Mar 12, 2023
View reviewed changes
core/src/main/java/org/apache/calcite/sql/SqlMapTypeNameSpec.java Outdated
writer.sep(","); // configures the writer
keyType.unparse(writer, leftPrec, rightPrec);
// Default is nullable.
if (keyType.getNullable() != null && !keyType.getNullable()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess checkerframework thinks the value could be changed between two invocations...
Probably it's better to extract keyType.getNullable() in a separate var to make checkerframework happy

@sonarcloud
Copy link

sonarcloud bot commented Mar 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

63.6% 63.6% Coverage
0.0% 0.0% Duplication

@julianhyde julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21
@sonarcloud
Copy link

sonarcloud bot commented Sep 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

63.6% 63.6% Coverage
0.0% 0.0% Duplication

@JiajunBernoulli JiajunBernoulli added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 9, 2023
@JiajunBernoulli JiajunBernoulli merged commit 6f79436 into apache:main Oct 1, 2023
32 checks passed
@MasseGuillaume
Copy link
Contributor

When you unparse with RelToSqlConverter it fails with:

[info]   java.lang.UnsupportedOperationException: Unsupported type when convertTypeToSpec: MAP
[info]   at org.apache.calcite.sql.type.SqlTypeUtil.convertTypeToSpec(SqlTypeUtil.java:1140)
[info]   at org.apache.calcite.sql.SqlDialect.getCastSpec(SqlDialect.java:848)
[info]   at io.narrative.nql.package$$anon$1.getCastSpec(package.scala:50)
[info]   at org.apache.calcite.rel.rel2sql.RelToSqlConverter.castNullType(RelToSqlConverter.java:486)
[info]   at org.apache.calcite.rel.rel2sql.RelToSqlConverter.visit(RelToSqlConverter.java:458)
[info]   at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[info]   at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
[info]   at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[info]   at java.base/java.lang.reflect.Method.invoke(Method.java:568)
[info]   at org.apache.calcite.util.ReflectUtil$2.invoke(ReflectUtil.java:532)

I think you need to add a branch for the Map type in

calcite/core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java

Line 1109 in 485a5d0

public static SqlDataTypeSpec convertTypeToSpec(RelDataType type,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snuyanzin snuyanzin Awaiting requested review from snuyanzin

Assignees
No one assigned
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JiajunBernoulli @MasseGuillaume @snuyanzin