[CALCITE-3429] AssertionError for user-defined table function with map argument by yanlin-Lynn · Pull Request #1521 · apache/calcite

yanlin-Lynn · 2019-10-18T12:24:06Z

Similar to #1519, using map in table function may cause exception.
Please refer to jira CALCITE-3429 for full stack trace.

To fix it, this PR make the follow changes:

Add map info from Java Map class to SqlTypeName.Map
SqlTypeName.Map is not a basic type, should not be used in factory method createSqlType, just like SqlTypeName.Array
Add coerce rule for SqlTypeName.MAP

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java

danny0405 · 2019-10-21T03:46:18Z

Can we avoid to use Refinement XXX for the title and commit message ? This kind of title really confused me a lot, instead, you should just specify that you did in the message, for example, for this patch, you can use "Support MAP type for JavaToSqlTypeConversionRules"

yanlin-Lynn · 2019-10-21T07:46:41Z

geode/src/test/java/org/apache/calcite/adapter/geode/rel/GeodeBookstoreTest.java

-            + "postalCode,country,phoneNumber,addressTag]\n"
-            + "primaryAddress=PDX[addressLine1,addressLine2,addressLine3,city,state,postalCode,"
-            + "country,phoneNumber,addressTag]\n")
+        .returns(new Consumer<ResultSet>() {


I update this case to make it pass.
But I'm not quite familiar with the logic, hopes someone more familiar with it can review this.

yanlin-Lynn · 2019-10-21T11:17:40Z

Can we avoid to use Refinement XXX for the title and commit message ? This kind of title really confused me a lot, instead, you should just specify that you did in the message, for example, for this patch, you can use "Support MAP type for JavaToSqlTypeConversionRules"

Sure, I'll avoid this next time.

danny0405 · 2019-10-22T02:24:46Z

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java

+     * Using ANY type as value type.
+     */
+    @Override public RelDataType getValueType() {
+      if (Map.class.isAssignableFrom(clazz)) {


Does any code use these 2 methods ?

yes, inSqlItemOperator.
If don't override these 2 methods, running org.apache.calcite.adapter.geode.rel.GeodeBookstoreTest will get NullPointerException

Caused by: java.lang.NullPointerException at org.apache.calcite.sql.fun.SqlItemOperator.getChecker(SqlItemOperator.java:103) at org.apache.calcite.sql.fun.SqlItemOperator.checkOperandTypes(SqlItemOperator.java:91) at org.apache.calcite.sql.SqlOperator.validateOperands(SqlOperator.java:432) at org.apache.calcite.sql.SqlOperator.deriveType(SqlOperator.java:518) at org.apache.calcite.sql.validate.SqlValidatorImpl$DeriveTypeVisitor.visit(SqlValidatorImpl.java:5639) at org.apache.calcite.sql.validate.SqlValidatorImpl$DeriveTypeVisitor.visit(SqlValidatorImpl.java:5626) at org.apache.calcite.sql.SqlCall.accept(SqlCall.java:139) at org.apache.calcite.sql.validate.SqlValidatorImpl.deriveTypeImpl(SqlValidatorImpl.java:1692) at org.apache.calcite.sql.validate.SqlValidatorImpl.deriveType(SqlValidatorImpl.java:1677) at org.apache.calcite.sql.validate.SqlValidatorImpl.expandSelectItem(SqlValidatorImpl.java:480) at org.apache.calcite.sql.validate.SqlValidatorImpl.validateSelectList(SqlValidatorImpl.java:4117) at org.apache.calcite.sql.validate.SqlValidatorImpl.validateSelect(SqlValidatorImpl.java:3396) at org.apache.calcite.sql.validate.SelectNamespace.validateImpl(SelectNamespace.java:60) at org.apache.calcite.sql.validate.AbstractNamespace.validate(AbstractNamespace.java:84) at org.apache.calcite.sql.validate.SqlValidatorImpl.validateNamespace(SqlValidatorImpl.java:1009) at org.apache.calcite.sql.validate.SqlValidatorImpl.validateQuery(SqlValidatorImpl.java:969) at org.apache.calcite.sql.SqlSelect.validate(SqlSelect.java:216)

jinxing64 · 2019-10-27T03:33:38Z

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java

+     */
+    @Override public RelDataType getKeyType() {
+      if (Map.class.isAssignableFrom(clazz)) {
+        return createSqlType(SqlTypeName.ANY);


Say the map is by Integer -> Integer
Why return SqlTypeName.ANY here ?

Because I cannot find a way to get the Integer parameter type when create type with Map class.
And use SqlTypeName.ANY can cover other types.

yanlin-Lynn · 2019-10-29T13:49:16Z

@ihuzenko Inspired by your PR (#1519), I checked using Map in table function, and throws exception too.
I refactor some part of your code, hope you can help to review when you have time.

yanlin-Lynn · 2019-11-01T09:23:50Z

hi, @danny0405
Thanks for reviewing.
I cannot see how this PR is blocked?
Can you give more details, if it need to update this PR.

…p argument (Wang Yanlin) 1. Add map info from Java Map class to SqlTypeName.Map 2. SqlTypeName.Map is not a basic type, should not be used in factory method "createSqlType", just like SqlTypeName.Array 3. Add coerce rule for SqlTypeName.MAP

danny0405 · 2019-12-05T02:57:17Z

Sorry for the delay, reviewing now ~

…p argument (Wang Yanlin) 1. Add map info from Java Map class to SqlTypeName.Map 2. SqlTypeName.Map is not a basic type, should not be used in factory method "createSqlType", just like SqlTypeName.Array 3. Add coerce rule for SqlTypeName.MAP Fix-up(by Danny): * Fix GeodeTable#query field type inference; * Fix GeodeUtils#convert for nested PdxInstance because in JavaTypeFactoryExtImpl#createPdxType, the nested PdxInstance is inferred as Map type(we can not describe nested JavaType correctly now). close apache#1521

…p argument (Wang Yanlin) 1. Add map info from Java Map class to SqlTypeName.Map 2. SqlTypeName.Map is not a basic type, should not be used in factory method "createSqlType", just like SqlTypeName.Array 3. Add coerce rule for SqlTypeName.MAP Fix-up(by Danny): * Fix GeodeTable#query field type inference; * Fix GeodeUtils#convert for nested PdxInstance because in JavaTypeFactoryExtImpl#createPdxType, the nested PdxInstance is inferred as Map type(we can not describe nested JavaType correctly now). close apache#1521 Change-Id: If1449d398b20914614dc573ee15d999b24223c3d

apache#124) …p argument (Wang Yanlin) 1. Add map info from Java Map class to SqlTypeName.Map 2. SqlTypeName.Map is not a basic type, should not be used in factory method "createSqlType", just like SqlTypeName.Array 3. Add coerce rule for SqlTypeName.MAP Fix-up(by Danny): * Fix GeodeTable#query field type inference; * Fix GeodeUtils#convert for nested PdxInstance because in JavaTypeFactoryExtImpl#createPdxType, the nested PdxInstance is inferred as Map type(we can not describe nested JavaType correctly now). close apache#1521 Change-Id: If1449d398b20914614dc573ee15d999b24223c3d

…p argument (Wang Yanlin) 1. Add map info from Java Map class to SqlTypeName.Map 2. SqlTypeName.Map is not a basic type, should not be used in factory method "createSqlType", just like SqlTypeName.Array 3. Add coerce rule for SqlTypeName.MAP Fix-up(by Danny): * Fix GeodeTable#query field type inference; * Fix GeodeUtils#convert for nested PdxInstance because in JavaTypeFactoryExtImpl#createPdxType, the nested PdxInstance is inferred as Map type(we can not describe nested JavaType correctly now). close apache#1521 Change-Id: If1449d398b20914614dc573ee15d999b24223c3d

yanlin-Lynn mentioned this pull request Oct 18, 2019

[CALCITE-3424] AssertionError thrown for user-defined table function with array argument #1519

Closed

yanlin-Lynn force-pushed the java-type branch from 03fae49 to b7d83ba Compare October 18, 2019 12:37

zabetak reviewed Oct 18, 2019

View reviewed changes

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java Outdated Show resolved Hide resolved

danny0405 reviewed Oct 19, 2019

View reviewed changes

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java Outdated Show resolved Hide resolved

yanlin-Lynn force-pushed the java-type branch 2 times, most recently from c56ce7a to e91f07d Compare October 21, 2019 03:04

yanlin-Lynn force-pushed the java-type branch 2 times, most recently from 7e0e168 to 4b9707b Compare October 21, 2019 07:29

yanlin-Lynn changed the title ~~[CALCITE-3429] Refinement for implementation of sql type factory~~ [CALCITE-3429] Support MAP type for JavaToSqlTypeConversionRules Oct 21, 2019

yanlin-Lynn force-pushed the java-type branch from 4b9707b to 9920103 Compare October 21, 2019 07:44

yanlin-Lynn commented Oct 21, 2019

View reviewed changes

danny0405 reviewed Oct 22, 2019

View reviewed changes

jinxing64 reviewed Oct 27, 2019

View reviewed changes

yanlin-Lynn changed the title ~~[CALCITE-3429] Support MAP type for JavaToSqlTypeConversionRules~~ [CALCITE-3429] AssertionError for user-defined table function with map argument Oct 29, 2019

yanlin-Lynn force-pushed the java-type branch 2 times, most recently from 5128659 to 5ff8d44 Compare October 29, 2019 13:45

danny0405 added the blocked-by-another-pr label Oct 31, 2019

danny0405 force-pushed the master branch 2 times, most recently from 80f411d to ca27fe9 Compare November 30, 2019 07:52

yanlin-Lynn force-pushed the java-type branch from 5ff8d44 to 8460d67 Compare December 2, 2019 09:34

yanlin-Lynn force-pushed the java-type branch from 8460d67 to 758079a Compare December 2, 2019 11:28

danny0405 added accepted-and-would-rework-soon This patch is overall ok, but need some refactoring by the committers. and removed blocked-by-another-pr labels Dec 5, 2019

danny0405 closed this in ff44204 Dec 6, 2019

yanlin-Lynn deleted the java-type branch December 6, 2019 06:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-3429] AssertionError for user-defined table function with map argument#1521

[CALCITE-3429] AssertionError for user-defined table function with map argument#1521
yanlin-Lynn wants to merge 1 commit intoapache:masterfrom
yanlin-Lynn:java-type

yanlin-Lynn commented Oct 18, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

danny0405 commented Oct 21, 2019

Uh oh!

yanlin-Lynn Oct 21, 2019

Uh oh!

yanlin-Lynn commented Oct 21, 2019

Uh oh!

danny0405 Oct 22, 2019

Uh oh!

yanlin-Lynn Oct 22, 2019

Uh oh!

jinxing64 Oct 27, 2019 •

edited

Loading

Uh oh!

yanlin-Lynn Oct 28, 2019

Uh oh!

yanlin-Lynn commented Oct 29, 2019

Uh oh!

yanlin-Lynn commented Nov 1, 2019

Uh oh!

danny0405 commented Dec 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

yanlin-Lynn commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danny0405 commented Oct 21, 2019

Uh oh!

yanlin-Lynn Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

yanlin-Lynn commented Oct 21, 2019

Uh oh!

danny0405 Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

yanlin-Lynn Oct 22, 2019

Choose a reason for hiding this comment

Uh oh!

jinxing64 Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanlin-Lynn Oct 28, 2019

Choose a reason for hiding this comment

Uh oh!

yanlin-Lynn commented Oct 29, 2019

Uh oh!

yanlin-Lynn commented Nov 1, 2019

Uh oh!

danny0405 commented Dec 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yanlin-Lynn commented Oct 18, 2019 •

edited

Loading

jinxing64 Oct 27, 2019 •

edited

Loading