Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-2016

ITEM + DOT operator does not work for array

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.15.0
    • Component/s: core
    • Labels:
      None

      Description

      Query with ITEM + DOT operator for array does not parse. Below is an example query and its stack trace.

      Query:

      SELECT employees[1].empno from dept_nested

      Stacktrace:

      java.lang.RuntimeException: Error while parsing query: SELECT employees[1].empno from dept_nested
      	at org.apache.calcite.sql.test.SqlTesterImpl.parseAndValidate(SqlTesterImpl.java:161)
      	at org.apache.calcite.sql.test.SqlTesterImpl.getResultType(SqlTesterImpl.java:148)
      	at org.apache.calcite.sql.test.SqlTesterImpl.checkResultType(SqlTesterImpl.java:204)
      	at org.apache.calcite.test.SqlValidatorTestCase$Sql.type(SqlValidatorTestCase.java:603)
      	at org.apache.calcite.test.SqlValidatorTest.testRecordTypeElided(SqlValidatorTest.java:7715)
      	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
      	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
      	at java.lang.reflect.Method.invoke(Method.java:498)
      	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
      	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
      	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
      	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
      	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
      	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
      	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
      	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
      	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
      	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
      	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
      	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
      	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
      	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
      	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
      	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:117)
      	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:42)
      	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:262)
      	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:84)
      	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
      	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
      	at java.lang.reflect.Method.invoke(Method.java:498)
      	at com.intellij.rt.execution.application.AppMain.main(AppMain.java:147)
      Caused by: org.apache.calcite.sql.parser.SqlParseException: Encountered "." at line 1, column 20.
      Was expecting one of:
          <EOF> 
          "ORDER" ...
          "LIMIT" ...
          "OFFSET" ...
          "FETCH" ...
          "FROM" ...
          "," ...
          "AS" ...
          <IDENTIFIER> ...
          <QUOTED_IDENTIFIER> ...
          <BACK_QUOTED_IDENTIFIER> ...
          <BRACKET_QUOTED_IDENTIFIER> ...
          <UNICODE_QUOTED_IDENTIFIER> ...
          "UNION" ...
          "INTERSECT" ...
          "EXCEPT" ...
          "MINUS" ...
          "NOT" ...
          "IN" ...
          "BETWEEN" ...
          "LIKE" ...
          "SIMILAR" ...
          "=" ...
          ">" ...
          "<" ...
          "<=" ...
          ">=" ...
          "<>" ...
          "!=" ...
          "+" ...
          "-" ...
          "*" ...
          "/" ...
          "||" ...
          "AND" ...
          "OR" ...
          "IS" ...
          "MEMBER" ...
          "SUBMULTISET" ...
          "CONTAINS" ...
          "OVERLAPS" ...
          "EQUALS" ...
          "PRECEDES" ...
          "SUCCEEDS" ...
          "MULTISET" ...
          "[" ...
          
      	at org.apache.calcite.sql.parser.impl.SqlParserImpl.convertException(SqlParserImpl.java:349)
      	at org.apache.calcite.sql.parser.impl.SqlParserImpl.normalizeException(SqlParserImpl.java:130)
      	at org.apache.calcite.sql.parser.SqlParser.parseQuery(SqlParser.java:138)
      	at org.apache.calcite.sql.test.SqlTesterImpl.parseQuery(SqlTesterImpl.java:168)
      	at org.apache.calcite.sql.test.SqlTesterImpl.parseAndValidate(SqlTesterImpl.java:159)
      	... 32 more
      Caused by: org.apache.calcite.sql.parser.impl.ParseException: Encountered "." at line 1, column 20.
      Was expecting one of:
          <EOF> 
          "ORDER" ...
          "LIMIT" ...
          "OFFSET" ...
          "FETCH" ...
          "FROM" ...
          "," ...
          "AS" ...
          <IDENTIFIER> ...
          <QUOTED_IDENTIFIER> ...
          <BACK_QUOTED_IDENTIFIER> ...
          <BRACKET_QUOTED_IDENTIFIER> ...
          <UNICODE_QUOTED_IDENTIFIER> ...
          "UNION" ...
          "INTERSECT" ...
          "EXCEPT" ...
          "MINUS" ...
          "NOT" ...
          "IN" ...
          "BETWEEN" ...
          "LIKE" ...
          "SIMILAR" ...
          "=" ...
          ">" ...
          "<" ...
          "<=" ...
          ">=" ...
          "<>" ...
          "!=" ...
          "+" ...
          "-" ...
          "*" ...
          "/" ...
          "||" ...
          "AND" ...
          "OR" ...
          "IS" ...
          "MEMBER" ...
          "SUBMULTISET" ...
          "CONTAINS" ...
          "OVERLAPS" ...
          "EQUALS" ...
          "PRECEDES" ...
          "SUCCEEDS" ...
          "MULTISET" ...
          "[" ...
          
      	at org.apache.calcite.sql.parser.impl.SqlParserImpl.generateParseException(SqlParserImpl.java:22153)
      	at org.apache.calcite.sql.parser.impl.SqlParserImpl.jj_consume_token(SqlParserImpl.java:21970)
      	at org.apache.calcite.sql.parser.impl.SqlParserImpl.SqlStmtEof(SqlParserImpl.java:800)
      	at org.apache.calcite.sql.parser.impl.SqlParserImpl.parseSqlStmtEof(SqlParserImpl.java:186)
      	at org.apache.calcite.sql.parser.SqlParser.parseQuery(SqlParser.java:131)
      	... 34 more
      

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.15.0 (2017-12-11).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.15.0 (2017-12-11).
          Hide
          julianhyde Julian Hyde added a comment -

          Further cleanup in bd141121.

          Show
          julianhyde Julian Hyde added a comment - Further cleanup in bd141121 .
          Hide
          julianhyde Julian Hyde added a comment -

          Fixed in a3dd90f1; thanks for the PR, Shuyi Chen!

          In a preceding commit I added a new method T SqlCallBinding.getOperandLiteralValue(int, Class<T>), which makes it easier to get the string value of a character literal operand, and refactored your code to use it.

          Show
          julianhyde Julian Hyde added a comment - Fixed in a3dd90f1 ; thanks for the PR, Shuyi Chen ! In a preceding commit I added a new method T SqlCallBinding.getOperandLiteralValue(int, Class<T>) , which makes it easier to get the string value of a character literal operand, and refactored your code to use it.
          Hide
          julianhyde Julian Hyde added a comment -

          Will try. If the PR is ready in time.

          Show
          julianhyde Julian Hyde added a comment - Will try. If the PR is ready in time.
          Hide
          suez1224 Shuyi Chen added a comment -

          Hi Julian Hyde, do you think if we can get this in 1.15 release?

          Show
          suez1224 Shuyi Chen added a comment - Hi Julian Hyde , do you think if we can get this in 1.15 release?
          Hide
          suez1224 Shuyi Chen added a comment - - edited

          Julian Hyde, I've updated the PR to change the parser to parse subfield as SqlIdentifier, and added a test to make sure that referring to an unknown field gives an error in SqlValidatorTest. Can you take another look at the PR(https://github.com/apache/calcite/pull/557)? Thanks.

          Show
          suez1224 Shuyi Chen added a comment - - edited Julian Hyde , I've updated the PR to change the parser to parse subfield as SqlIdentifier, and added a test to make sure that referring to an unknown field gives an error in SqlValidatorTest. Can you take another look at the PR( https://github.com/apache/calcite/pull/557 )? Thanks.
          Hide
          julianhyde Julian Hyde added a comment -

          One thing you could do is to keep the field name as a character literal, as you have it, and fix the unparse method. However, that doesn't validate the field name. So, better, I think, is to change the validation of the DOT operator so that it doesn't try to validate the field name argument as an identifier. SqlAsOperator.validateCall already deals with this problem.

          Can you add a test to SqlValidatorTest to make sure that referring to an unknown field gives an error?

          Show
          julianhyde Julian Hyde added a comment - One thing you could do is to keep the field name as a character literal, as you have it, and fix the unparse method. However, that doesn't validate the field name. So, better, I think, is to change the validation of the DOT operator so that it doesn't try to validate the field name argument as an identifier. SqlAsOperator.validateCall already deals with this problem. Can you add a test to SqlValidatorTest to make sure that referring to an unknown field gives an error?
          Hide
          suez1224 Shuyi Chen added a comment -

          Hi Julian Hyde, you are right, I did try to parse 'myfield' as a SqlIdentifier. However, I am hitting the problem of subfield validation. Basically, 'myfield' will be parsed as a standalone SqlIdentifier, and in the validation process, it will try to fullyQualify the SqlIdentifier, which the validation scope won't be able to find 'myfield' as it's nested under a top level field.

          The approach I was taking basically treat DOT similar to ITEM. In ITEM, you don't need to check the second operand as it's a literal. So the current implementation
          will bypass checking if the subfield actually exist in the record.

          Also, I think about extend SqlIdentifier(see CompoundIdentifier()) itself to support both ITEM and DOT. However, in that case, you can't support complex expression in the ITEM operand, e.g.
          `A`.`B`[(1+udf(`C`)].`D`.

          What's your recommendation? Thanks.

          Show
          suez1224 Shuyi Chen added a comment - Hi Julian Hyde , you are right, I did try to parse 'myfield' as a SqlIdentifier. However, I am hitting the problem of subfield validation. Basically, 'myfield' will be parsed as a standalone SqlIdentifier, and in the validation process, it will try to fullyQualify the SqlIdentifier, which the validation scope won't be able to find 'myfield' as it's nested under a top level field. The approach I was taking basically treat DOT similar to ITEM. In ITEM, you don't need to check the second operand as it's a literal. So the current implementation will bypass checking if the subfield actually exist in the record. Also, I think about extend SqlIdentifier(see CompoundIdentifier()) itself to support both ITEM and DOT. However, in that case, you can't support complex expression in the ITEM operand, e.g. `A`.`B` [(1+udf(`C`)] .`D`. What's your recommendation? Thanks.
          Hide
          julianhyde Julian Hyde added a comment -

          Shuyi Chen, With your parser changes, you have made select myrecord . 'myfield' from mytable into valid SQL but I think it should not be. Do you agree?

          Show
          julianhyde Julian Hyde added a comment - Shuyi Chen , With your parser changes, you have made select myrecord . 'myfield' from mytable into valid SQL but I think it should not be. Do you agree?
          Hide
          julianhyde Julian Hyde added a comment -

          I know you're trying to extend the grammar but you have made it accept string literals, e.g.

          select myrecord . 'myfield' from mytable
          

          whereas we only need identifiers, e.g.

          select myrecord . myfield from mytable
          

          or quoted identifiers, e.g.

          select myrecord . "myfield" from mytable
          

          I think perhaps when you see a dot operator you should make the right-hand side into a SqlIdentifier rather than a SqlLiteral.

          Show
          julianhyde Julian Hyde added a comment - I know you're trying to extend the grammar but you have made it accept string literals, e.g. select myrecord . 'myfield' from mytable whereas we only need identifiers, e.g. select myrecord . myfield from mytable or quoted identifiers, e.g. select myrecord . "myfield" from mytable I think perhaps when you see a dot operator you should make the right-hand side into a SqlIdentifier rather than a SqlLiteral.
          Hide
          suez1224 Shuyi Chen added a comment -

          Julian Hyde

          • Yes, I am trying to extend the grammar to support nested array access.
          • Use getOperandLiteralValue instead.
          • fixed according to comment.
          Show
          suez1224 Shuyi Chen added a comment - Julian Hyde Yes, I am trying to extend the grammar to support nested array access. Use getOperandLiteralValue instead. fixed according to comment.
          Hide
          julianhyde Julian Hyde added a comment -

          Also:

          • Your type derivation is slightly wrong. You are deriving the type of record.field as always nullable. It should be nullable only if record is nullable or field is nullable.
          Show
          julianhyde Julian Hyde added a comment - Also: Your type derivation is slightly wrong. You are deriving the type of record.field as always nullable. It should be nullable only if record is nullable or field is nullable.
          Hide
          julianhyde Julian Hyde added a comment -

          Reviewing https://github.com/apache/calcite/pull/557/commits/3ba0b6caa8c60759bf42b985eae0249359d37a0c:

          • In Parser.jj, why is there a section for <QUOTED_ID>? Are you trying to extend the grammar?
          • getStringLiteralOperand is deprecated
          Show
          julianhyde Julian Hyde added a comment - Reviewing https://github.com/apache/calcite/pull/557/commits/3ba0b6caa8c60759bf42b985eae0249359d37a0c: In Parser.jj, why is there a section for <QUOTED_ID> ? Are you trying to extend the grammar? getStringLiteralOperand is deprecated
          Hide
          suez1224 Shuyi Chen added a comment - - edited

          Julian Hyde Could you please help take a look at this PR, https://github.com/apache/calcite/pull/557?

          Show
          suez1224 Shuyi Chen added a comment - - edited Julian Hyde Could you please help take a look at this PR, https://github.com/apache/calcite/pull/557?
          Hide
          suez1224 Shuyi Chen added a comment -

          Thanks, Julian Hyde and Jacques Nadeau. I'll take a look and try to fix it soon.

          Show
          suez1224 Shuyi Chen added a comment - Thanks, Julian Hyde and Jacques Nadeau . I'll take a look and try to fix it soon.
          Show
          jnadeau Jacques Nadeau added a comment - You may find inspiration from: https://github.com/apache/drill/blob/master/exec/java-exec/src/main/codegen/includes/compoundIdentifier.ftl https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/CompoundIdentifierConverter.java https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/DrillCompoundIdentifier.java
          Hide
          julianhyde Julian Hyde added a comment -

          Yes, please see if you can develop a fix. Best place to start is to add a test to SqlParserTest. Then you probably should modify Parser.jj (if you're lucky it will be a change to the grammar; if unlucky it will be related to SqlParserUtil.toTree).

          Show
          julianhyde Julian Hyde added a comment - Yes, please see if you can develop a fix. Best place to start is to add a test to SqlParserTest. Then you probably should modify Parser.jj (if you're lucky it will be a change to the grammar; if unlucky it will be related to SqlParserUtil.toTree).
          Hide
          suez1224 Shuyi Chen added a comment -

          Julian Hyde I take a look, seems like the problem is in the Parser, the ITEM operator does not expect any subsequent operators chaining after it. I can try to help contribute the fix if needed.

          Show
          suez1224 Shuyi Chen added a comment - Julian Hyde I take a look, seems like the problem is in the Parser, the ITEM operator does not expect any subsequent operators chaining after it. I can try to help contribute the fix if needed.

            People

            • Assignee:
              julianhyde Julian Hyde
              Reporter:
              suez1224 Shuyi Chen
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development