[CALCITE-2417] Fix ClassCastException in RelToSqlConverter with structs by BenoitHanotte · Pull Request #761 · apache/calcite

BenoitHanotte · 2018-07-17T16:32:00Z

Trying to convert the pysical plan of a select * query on a table with a nested struct throws ClassCastException as RexInputRef cannot be cast to RexCorrelVariable.
This is due to the cast to RexCorrelVariable not being done on the referenced expression of RexInputRef accessible through RexFieldAccess.getReferenceExpr()

I am not sure how you would advise testing such a case? In order to isolate the issue I had to recreate a custom schema with a nested struct, however this is adds a lot of boiler plate.

michaelmior · 2018-07-17T17:30:35Z

Overall LGTM. Can you rename RelToSqlNestedStruct to RelToSqlNestedStructTest? It would also probably be a good idea to validate the result of RelToSqlConverter.

BenoitHanotte · 2018-07-18T07:26:32Z

Hello @michaelmior, thanks for the feedback :)
I renamed the test class and updated the test case to assert the validity of the sql string.

zabetak · 2018-07-18T08:49:26Z

Hi @BenoitHanotte, for creating appropriate tests I believe you can add a few test methods in RelToSqlConverterTest. A schema with some minimal nesting that may be sufficient for your case exists in CalciteAssert.SchemaSpec.HR.

If you need a Schema with more nesting in order to test your feature wait for PR762.

BenoitHanotte · 2018-07-18T09:42:41Z

Thanks @zabetak , indeed the depts table has a nested struct that allows testing this fix with a much reduced boiler plate.
I updated my PR to add a test case to RelToSqlConverterTest and remove the RelToSqlConverterNestedStructsTest file, it is much cleaner this way :)

BenoitHanotte · 2018-07-18T12:12:32Z

Unfortunately I discovered that this fix is not giving correct names for the nested fields:

SELECT * FROM depts

gives the following query:

SELECT deptno, name, employees, location, location AS location FROM hr.depts

I'll have alook at it this afternoon but from my limited understanding of the code, we'll have to make sure that RexFieldAccess is actually going through the correlation path to keep track of the reference

BenoitHanotte · 2018-07-18T17:22:48Z

@zabetak @michaelmior I updated my PR, it seems that in addition to the cast issue we were not correctly mapping the field index to the RelDataTypeField of the row when the row type is nested.

For instance, for a RecordType(INT a, RecordType(INT b), we need to build an ordered list such as

0: RelDataTypeField of a
1: RelDataTypeField of a.b

This way we can get the RelDataTypeField for given field index in an AliasContext

I added a few test cases to ensure that names were correctly retrieved for nested fields.

michaelmior · 2018-07-18T19:10:14Z

core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java

        for (Function<RelNode, RelNode> transform : transforms) {
          rel = transform.apply(rel);
        }
+        System.out.println(RelOptUtil.toString(rel));


Minor, but looks like this was for debugging and should be removed.

Sorry, I forgot to remove it

zabetak · 2018-07-18T22:31:11Z

core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java

+          final RexCorrelVariable variable = (RexCorrelVariable) access.getReferenceExpr();
+          final Context aliasContext = correlTableMap.get(variable.id);
+          return aliasContext.field(access.getField().getIndex());
+        default:


The code assumes that FIELD_ACCESS wraps always a CORREL_VARIABLE or INPUT_REF. I have the impression that this code will lead again to a ClassCastException just a bit later (e.g., for a doubly nested struct where there is a RexFieldAccess(RexFieldAccess(RexInputRef)).

You're right, I thought that it could only go down one level but after adding tests for multiple nested levels I see that it can happen, I am updating the PR to support this and add tests for this case

BenoitHanotte · 2018-07-19T11:13:03Z

I updated the PR to do the following:

Support more than one level of RexFieldAccess (use a stack called accesses to store the RexFieldAccess levels, get the referenced SqlNodeand then poll from the stack to add the name components)
Add tests on a schema containing more than one level of nesting and more than one nested column

I haven't been able to find a more elegant solution to the correlation variable case as I don't seem to be able to get the field index without having access to the last level of RexFieldAccess, hence the inner switch case to handle this particular case.

I tested this patch on our production schema and it seems to have solved the issue.

Trying to convert the pysical plan of a select * query on a table with a nested struct throws ClassCastException as RexInputRef cannot be cast to RexCorrelVariable. This is due to the cast to RexCorrelVariable not being done on the referenced expression of RexInputRef accessible through RexFieldAccess.getReferenceExpr()

BenoitHanotte · 2018-07-25T11:56:37Z

Hello @michaelmior @zabetak would you have time for a review of this PR so that we can see whether this is the right approach or not? Thanks!

michaelmior · 2018-07-25T17:24:52Z

LGTM, merged! Sorry for the delay. I was waiting for the latest release to go through and this slipped off my radar. Thanks for the contribution :)

…ts (Benoit Hanotte) Trying to convert the pysical plan of a select * query on a table with a nested struct throws ClassCastException as RexInputRef cannot be cast to RexCorrelVariable. This is due to the cast to RexCorrelVariable not being done on the referenced expression of RexInputRef accessible through RexFieldAccess.getReferenceExpr() Close apache#761

…ts (Benoit Hanotte) - This patch changes RelToSqlConvertedTest framework which is needed by the calcite-3138 Trying to convert the pysical plan of a select * query on a table with a nested struct throws ClassCastException as RexInputRef cannot be cast to RexCorrelVariable. This is due to the cast to RexCorrelVariable not being done on the referenced expression of RexInputRef accessible through RexFieldAccess.getReferenceExpr() Close apache#761 Change-Id: I43416d770e757deb521e4a1cebe003fcc73df7ee

BenoitHanotte force-pushed the CALCITE-2417 branch 2 times, most recently from f3dee40 to ff2aaf8 Compare July 18, 2018 06:57

BenoitHanotte force-pushed the CALCITE-2417 branch from ff2aaf8 to d0ce4b4 Compare July 18, 2018 09:40

BenoitHanotte force-pushed the CALCITE-2417 branch from d0ce4b4 to cb34cc1 Compare July 18, 2018 09:43

BenoitHanotte force-pushed the CALCITE-2417 branch from cb34cc1 to d917a53 Compare July 18, 2018 17:02

michaelmior reviewed Jul 18, 2018

View reviewed changes

zabetak reviewed Jul 18, 2018

View reviewed changes

BenoitHanotte force-pushed the CALCITE-2417 branch 2 times, most recently from 8e82059 to f9d609c Compare July 19, 2018 10:27

BenoitHanotte force-pushed the CALCITE-2417 branch from f9d609c to bb977dd Compare July 19, 2018 11:17

asfgit closed this in 3c40d86 Jul 25, 2018

Conversation

BenoitHanotte commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelmior commented Jul 17, 2018

Uh oh!

BenoitHanotte commented Jul 18, 2018

Uh oh!

zabetak commented Jul 18, 2018

Uh oh!

BenoitHanotte commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenoitHanotte commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenoitHanotte commented Jul 18, 2018

Uh oh!

michaelmior Jul 18, 2018

Choose a reason for hiding this comment

Uh oh!

BenoitHanotte Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

zabetak Jul 18, 2018

Choose a reason for hiding this comment

Uh oh!

BenoitHanotte Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

BenoitHanotte commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenoitHanotte commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelmior commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BenoitHanotte commented Jul 17, 2018 •

edited

Loading

BenoitHanotte commented Jul 18, 2018 •

edited

Loading

BenoitHanotte commented Jul 18, 2018 •

edited

Loading

BenoitHanotte commented Jul 19, 2018 •

edited

Loading

BenoitHanotte commented Jul 25, 2018 •

edited

Loading