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

Not (C='a' or C='b') as well as Not (C='a' and C='b') causes NPE

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.0-incubating
    • Fix Version/s: 1.6.0
    • Component/s: None
    • Labels:
      None

      Description

      A where clause like Not (C='a' or C='b') causes NPE if C has NULL value.

      The generated code snippet looks like:

      /*  65 */           public boolean moveNext() {
      /*  66 */             while (inputEnumerator.moveNext()) {
      /*  67 */               final Object[] current = (Object[]) inputEnumerator.current();
      /*  68 */               final String inp21_ = current[21] == null ? (String) null : current[21].toString();
      /*  69 */               final Boolean v = inp21_ == null ? (Boolean) null : Boolean.valueOf(org.apache.calcite.runtime.SqlFunctions.eq(inp21_, "A"));
      /*  70 */               final Boolean v0 = inp21_ == null ? (Boolean) null : Boolean.valueOf(org.apache.calcite.runtime.SqlFunctions.eq(inp21_, "B"));
      /*  71 */               if (!(v == null ? (v0 == null || !v0 ? (Boolean) null : Boolean.TRUE) : v ? Boolean.TRUE : v0)) {
      /*  72 */                 return true;
      /*  73 */               }
      /*  74 */             }
      /*  75 */             return false;
      /*  76 */           }
      

      And NPE is thrown at line #71 if inp21_ is null.

      Stacktrace:

      Caused by: java.lang.NullPointerException
      	at Baz$1$1.moveNext(ANONYMOUS.java:71)
      	at org.apache.calcite.linq4j.EnumerableDefaults.groupBy_(EnumerableDefaults.java:737)
      	at org.apache.calcite.linq4j.EnumerableDefaults.groupBy(EnumerableDefaults.java:677)
      	at org.apache.calcite.linq4j.DefaultEnumerable.groupBy(DefaultEnumerable.java:301)
      	at Baz.bind(Baz.java:95)
      	at org.apache.calcite.jdbc.CalcitePrepare$CalciteSignature.enumerable(CalcitePrepare.java:281)
      	at org.apache.calcite.jdbc.CalciteConnectionImpl.enumerable(CalciteConnectionImpl.java:235)
      	at org.apache.calcite.jdbc.CalciteMetaImpl.createIterable(CalciteMetaImpl.java:533)
      	at org.apache.calcite.avatica.AvaticaResultSet.execute(AvaticaResultSet.java:184)
      	at org.apache.calcite.jdbc.CalciteResultSet.execute(CalciteResultSet.java:63)
      	at org.apache.calcite.jdbc.CalciteResultSet.execute(CalciteResultSet.java:42)
      	at org.apache.calcite.avatica.AvaticaConnection$1.execute(AvaticaConnection.java:473)
      	at org.apache.calcite.jdbc.CalciteMetaImpl.prepareAndExecute(CalciteMetaImpl.java:566)
      	at org.apache.calcite.avatica.AvaticaConnection.prepareAndExecuteInternal(AvaticaConnection.java:477)
      	at org.apache.calcite.avatica.AvaticaStatement.executeInternal(AvaticaStatement.java:109)
      	... 29 more
      

        Issue Links

          Activity

          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -
            /** Tests CALCITE-980: Not (C='a' or C='b') causes NPE */
            @Test public void testWhereNotOrNullable() {
              CalciteAssert.that()
                  .with(CalciteAssert.Config.REGULAR)
                  .query("with tst(c) as (values('a'),('b'),('c'),(cast(null as varchar)))"
                          + " select * from tst where not (c='a' or c='b')")
                  .returns(
                      "not sure yet\n");
            }
          

          The generated code boils down to:

                Boolean aBoolean = v0 == null || !v0 ? (Boolean) null : Boolean.TRUE;
                Boolean aBoolean1 = v ? Boolean.TRUE : v0;
                Boolean aBoolean2 = v == null ? aBoolean : aBoolean1;
                if (!aBoolean2) { // <-- NPE here
          

          So either NOT is improperly translated (without null check) or something else is going on.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - /** Tests CALCITE-980: Not (C='a' or C='b') causes NPE */ @Test public void testWhereNotOrNullable() { CalciteAssert.that() .with(CalciteAssert.Config.REGULAR) .query( "with tst(c) as (values('a'),('b'),('c'),( cast ( null as varchar)))" + " select * from tst where not (c='a' or c='b')" ) .returns( "not sure yet\n" ); } The generated code boils down to: Boolean aBoolean = v0 == null || !v0 ? ( Boolean ) null : Boolean .TRUE; Boolean aBoolean1 = v ? Boolean .TRUE : v0; Boolean aBoolean2 = v == null ? aBoolean : aBoolean1; if (!aBoolean2) { // <-- NPE here So either NOT is improperly translated (without null check) or something else is going on.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment - - edited

          The problem is with OR implementor in nullsAs=TRUE mode: https://github.com/apache/calcite/blob/d012b245e4e040ae94bb3d7045cf4d71af8ac279/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java#L476-L488

          The following monkey-patch solves the particular NPE. However, I would like OR/AND implementors to be reviewed & simplified.

          iff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
          index 07f3f43..fb4dda6 100644
          --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
          +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
          @@ -473,7 +473,7 @@ public Expression implement(
                     if (!nullable(call2, 0) && !nullable(call2, 1)) {
                       return Expressions.orElse(t0, t1);
                     }
          -          return optimize(
          +          return optimize(nullAs.handle(
                         Expressions.condition(
                             Expressions.equal(t0, NULL_EXPR),
                             Expressions.condition(
          @@ -485,7 +485,7 @@ public Expression implement(
                             Expressions.condition(
                                 Expressions.not(t0),
                                 t1,
          -                      BOXED_TRUE_EXPR)));
          +                      BOXED_TRUE_EXPR))));
                   }
                 };
               case NOT:
          diff --git a/core/src/test/java/org/apache/calcite/test/JdbcTest.java b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
          index 3709eba..c1b4290 100644
          --- a/core/src/test/java/org/apache/calcite/test/JdbcTest.java
          +++ b/core/src/test/java/org/apache/calcite/test/JdbcTest.java
          @@ -4277,6 +4277,16 @@ private void startOfGroupStep3(String startOfGroup) {
                       "empid=100; deptno=10; name=Bill; salary=10000.0; commission=1000\n");
             }
          
          +  /** Tests CALCITE-980: Not (C='a' or C='b') causes NPE */
          +  @Test public void testWhereNotOrNullable() {
          +    CalciteAssert.that()
          +        .with(CalciteAssert.Config.REGULAR)
          +        .query("with tst(c) as (values('a'),('b'),('c'),(cast(null as varchar)))"
          +                + " select * from tst where not (c='a' or c='b')")
          +        .returns(
          +            "qwert\n");
          +  }
          +
             /** Tests the LIKE operator. */
             @Test public void testLike() {
               CalciteAssert.that()
          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - - edited The problem is with OR implementor in nullsAs=TRUE mode: https://github.com/apache/calcite/blob/d012b245e4e040ae94bb3d7045cf4d71af8ac279/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java#L476-L488 The following monkey-patch solves the particular NPE. However, I would like OR/AND implementors to be reviewed & simplified. iff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java index 07f3f43..fb4dda6 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java @@ -473,7 +473,7 @@ public Expression implement( if (!nullable(call2, 0) && !nullable(call2, 1)) { return Expressions.orElse(t0, t1); } - return optimize( + return optimize(nullAs.handle( Expressions.condition( Expressions.equal(t0, NULL_EXPR), Expressions.condition( @@ -485,7 +485,7 @@ public Expression implement( Expressions.condition( Expressions.not(t0), t1, - BOXED_TRUE_EXPR))); + BOXED_TRUE_EXPR)))); } }; case NOT: diff --git a/core/src/test/java/org/apache/calcite/test/JdbcTest.java b/core/src/test/java/org/apache/calcite/test/JdbcTest.java index 3709eba..c1b4290 100644 --- a/core/src/test/java/org/apache/calcite/test/JdbcTest.java +++ b/core/src/test/java/org/apache/calcite/test/JdbcTest.java @@ -4277,6 +4277,16 @@ private void startOfGroupStep3(String startOfGroup) { "empid=100; deptno=10; name=Bill; salary=10000.0; commission=1000\n"); } + /** Tests CALCITE-980: Not (C='a' or C='b') causes NPE */ + @Test public void testWhereNotOrNullable() { + CalciteAssert.that() + .with(CalciteAssert.Config.REGULAR) + .query("with tst(c) as (values('a'),('b'),('c'),(cast(null as varchar)))" + + " select * from tst where not (c='a' or c='b')") + .returns( + "qwert\n"); + } + /** Tests the LIKE operator. */ @Test public void testLike() { CalciteAssert.that()
          Hide
          julianhyde Julian Hyde added a comment -

          Thanks for the monkey patch Vladimir Sitnikov. I agree that that code needs some more thought.

          I don't think we've thoroughly tested all operators with all possible values of NullAs.

          I'm not confident that we do the right thing if a common sub-expression occurs in two different branches. E.g. where ((c = 1 and d = 2) or (e = 1 and d = 2)) or similar. Note that d = 2 may be in the environment of expressions that are "known" but we might be in a branch where it has never been assigned.

          Now the question is whether we check in the "monkey path" before we have time to do it properly.

          Show
          julianhyde Julian Hyde added a comment - Thanks for the monkey patch Vladimir Sitnikov . I agree that that code needs some more thought. I don't think we've thoroughly tested all operators with all possible values of NullAs. I'm not confident that we do the right thing if a common sub-expression occurs in two different branches. E.g. where ((c = 1 and d = 2) or (e = 1 and d = 2)) or similar. Note that d = 2 may be in the environment of expressions that are "known" but we might be in a branch where it has never been assigned. Now the question is whether we check in the "monkey path" before we have time to do it properly.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment - - edited

          Here's more sophisticated patch: https://github.com/apache/calcite/pull/169

          I'm very much depressed by usage of assertTrue in RexImplicationCheckerTest: it just bails out without giving proper error message.

          I'll try adding where ((c = 1 and d = 2) or (e = 1 and d = 2)) or similar test.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - - edited Here's more sophisticated patch: https://github.com/apache/calcite/pull/169 I'm very much depressed by usage of assertTrue in RexImplicationCheckerTest : it just bails out without giving proper error message. I'll try adding where ((c = 1 and d = 2) or (e = 1 and d = 2)) or similar test.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment - - edited

          I think 169 is ready.

          I'd be happy if someone reviews that.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - - edited I think 169 is ready. I'd be happy if someone reviews that.
          Hide
          julianhyde Julian Hyde added a comment -

          A few comments about code style:

          • RexImpTable lines 455, 499 both open two parentheses on the same line. I tend to avoid that, because it's ambiguous what the indentation of the next line should be.
          • In testWhereOrAndNullable, put the reference to the JIRA case in a URL, the way say testTableMacroMap does.

          I really appreciate you adding lots of tests to test around the problem but JdbcTest is becoming bloated. I favor putting extra tests of this kind into one of the .oq tests. Could you put all test cases into say misc.oq or create a new expression.oq, and remove all but testWhereOrAndNullable from JdbcTest.

          The fix and in particular the generated code looks good - nice work. +1 when you've resolved the above nits.

          Show
          julianhyde Julian Hyde added a comment - A few comments about code style: RexImpTable lines 455, 499 both open two parentheses on the same line. I tend to avoid that, because it's ambiguous what the indentation of the next line should be. In testWhereOrAndNullable, put the reference to the JIRA case in a URL, the way say testTableMacroMap does. I really appreciate you adding lots of tests to test around the problem but JdbcTest is becoming bloated. I favor putting extra tests of this kind into one of the .oq tests. Could you put all test cases into say misc.oq or create a new expression.oq, and remove all but testWhereOrAndNullable from JdbcTest. The fix and in particular the generated code looks good - nice work. +1 when you've resolved the above nits.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          lots of tests to test around the problem but JdbcTest is becoming bloated

          I would extract test methods to separate class.

          I prefer having classes since that allows selective test execution, it is simpler in terms of setting break point.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - lots of tests to test around the problem but JdbcTest is becoming bloated I would extract test methods to separate class. I prefer having classes since that allows selective test execution, it is simpler in terms of setting break point.
          Hide
          julianhyde Julian Hyde added a comment -

          But consider the likelihood that someone will need to debug those tests in future versus the likelihood that someone will need to maintain them. I have seen several database projects (including my own, Mondrian) crushed under the burden of a large test suite. This is why I want to move as many tests as possible to Quidem format, which is easier to maintain. If anyone needed to debug one of those cases in future they could easily convert back to junit format.

          Show
          julianhyde Julian Hyde added a comment - But consider the likelihood that someone will need to debug those tests in future versus the likelihood that someone will need to maintain them. I have seen several database projects (including my own, Mondrian) crushed under the burden of a large test suite. This is why I want to move as many tests as possible to Quidem format, which is easier to maintain. If anyone needed to debug one of those cases in future they could easily convert back to junit format.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          But consider the likelihood that someone will need to debug those tests in future versus the likelihood that someone will need to maintain them.

          Whoever would touch generation of "and/or", would probably get bitten by the test.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - But consider the likelihood that someone will need to debug those tests in future versus the likelihood that someone will need to maintain them. Whoever would touch generation of "and/or", would probably get bitten by the test.
          Hide
          julianhyde Julian Hyde added a comment -

          Generation of AND and OR probably won't be touched for a while. (Now you've fixed this one there are no known bugs, and likely very few unknown ones.) I find that when I maintain a component, unless I'm completely rewriting it, I'm unlikely to break more than 1 in 10 existing tests. Unit tests are scaffolding when you're first developing a feature, but after that they become a safety net that is very rarely used but still needs to be there.

          Show
          julianhyde Julian Hyde added a comment - Generation of AND and OR probably won't be touched for a while. (Now you've fixed this one there are no known bugs, and likely very few unknown ones.) I find that when I maintain a component, unless I'm completely rewriting it, I'm unlikely to break more than 1 in 10 existing tests. Unit tests are scaffolding when you're first developing a feature, but after that they become a safety net that is very rarely used but still needs to be there.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          I'll check if I could get familiar with Quidem.

          I think the next step would be exhaustive testing of "nullable vs non-nullable combinations".

          I'm still not sure if "inferred return types" are right.
          `foldOr` / `foldAnd` return non-boxed primitives, while fallback and/or return boxed (to return null somehow).

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - I'll check if I could get familiar with Quidem. I think the next step would be exhaustive testing of "nullable vs non-nullable combinations". I'm still not sure if "inferred return types" are right. `foldOr` / `foldAnd` return non-boxed primitives, while fallback and/or return boxed (to return null somehow).
          Hide
          julianhyde Julian Hyde added a comment -

          Anyway, I'm not going to veto your change. You're welcome to move the tests into another Java class, or keep them in JdbcTest. Or SqlOperatorBaseTest, which some nice facilities for running the same tests against different engines. I don't think there's a "right answer" but I wanted to make you aware of the alternatives.

          Yes, it's hard to test the nullable and non-nullable combinations, especially when you include all of the other operators, such as CASE and IN and NOT.

          Regarding boxed return type. When a boolean is used in a WHERE clause it is implicitly wrapped in "IS TRUE" i.e. null is treated as false, so we take great pains to keep booleans primitive. That optimization is great for end users, but makes it more difficult to test for null behavior.

          Show
          julianhyde Julian Hyde added a comment - Anyway, I'm not going to veto your change. You're welcome to move the tests into another Java class, or keep them in JdbcTest. Or SqlOperatorBaseTest, which some nice facilities for running the same tests against different engines. I don't think there's a "right answer" but I wanted to make you aware of the alternatives. Yes, it's hard to test the nullable and non-nullable combinations, especially when you include all of the other operators, such as CASE and IN and NOT. Regarding boxed return type. When a boolean is used in a WHERE clause it is implicitly wrapped in "IS TRUE" i.e. null is treated as false, so we take great pains to keep booleans primitive. That optimization is great for end users, but makes it more difficult to test for null behavior.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          SqlOperatorBaseTest, which some nice facilities for running the same tests against different engines. I don't think there's a "right answer" but I wanted to make you aware of the alternatives.

          I thought of something like SqlOperatorBaseTest. I was just lazy as having plain old sql is easier to validate against you-know-what-db.

          When a boolean is used in a WHERE clause it is implicitly wrapped in "IS TRUE" i.e. null is treated as false, so we take great pains to keep booleans primitive.

          Is it driven by semantics or by just a mere optimization?

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - SqlOperatorBaseTest, which some nice facilities for running the same tests against different engines. I don't think there's a "right answer" but I wanted to make you aware of the alternatives. I thought of something like SqlOperatorBaseTest. I was just lazy as having plain old sql is easier to validate against you-know-what-db. When a boolean is used in a WHERE clause it is implicitly wrapped in "IS TRUE" i.e. null is treated as false, so we take great pains to keep booleans primitive. Is it driven by semantics or by just a mere optimization?
          Hide
          julianhyde Julian Hyde added a comment -

          It's an optimization, constrained by semantics. Consider WHERE NOT x = 5, which is implicitly WHERE (NOT (x = 5)) IS TRUE, and note that if x is null, x = 5 is unknown, NOT (x = 5)) is unknown, and (NOT (x = 5) IS TRUE) is false.

          The simple way to achieve the semantics is to use a Boolean Java variable that has a null value to represent unknown. But we never generate a null value. We know that if x is null the ultimate result will be false, so we pretend that x = 5 evaluates to true. That's what the "treat null as" semantics is all about.

          Show
          julianhyde Julian Hyde added a comment - It's an optimization, constrained by semantics. Consider WHERE NOT x = 5 , which is implicitly WHERE (NOT (x = 5)) IS TRUE , and note that if x is null, x = 5 is unknown, NOT (x = 5)) is unknown, and (NOT (x = 5) IS TRUE) is false. The simple way to achieve the semantics is to use a Boolean Java variable that has a null value to represent unknown. But we never generate a null value. We know that if x is null the ultimate result will be false, so we pretend that x = 5 evaluates to true. That's what the "treat null as" semantics is all about.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          That makes sense.
          So "nullable" translation of and/or could happen when calling a user defined function that takes boolean.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - That makes sense. So "nullable" translation of and/or could happen when calling a user defined function that takes boolean.
          Hide
          julianhyde Julian Hyde added a comment -

          Yes, provided the UDF is strict. If it is strict we will generate something like return arg == null ? null : myUdf(arg.booleanValue()). If it is not strict we should give an error that you are passing a null value to an not-null parameter.

          Show
          julianhyde Julian Hyde added a comment - Yes, provided the UDF is strict. If it is strict we will generate something like return arg == null ? null : myUdf(arg.booleanValue()) . If it is not strict we should give an error that you are passing a null value to an not-null parameter.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          If it is not strict we should give an error that you are passing a null value to an not-null parameter.

          Why's that?
          Why can't UDF just accept null value and act as it desires?

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - If it is not strict we should give an error that you are passing a null value to an not-null parameter. Why's that? Why can't UDF just accept null value and act as it desires?
          Hide
          julianhyde Julian Hyde added a comment -

          If its parameter is boolean and not java.lang.Boolean then it cannot accept null values.

          Show
          julianhyde Julian Hyde added a comment - If its parameter is boolean and not java.lang.Boolean then it cannot accept null values.
          Hide
          julianhyde Julian Hyde added a comment -

          Vladimir Sitnikov, Is this good to go? If so, please commit it.

          Show
          julianhyde Julian Hyde added a comment - Vladimir Sitnikov , Is this good to go? If so, please commit it.
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          Julian Hyde, it is not yet ready I've reworked PR and identified a couple of new bugs:
          1) "a or b" does not return (null; true) row somehow. <-- I believe this is a showstopper. I'm not sure where the bug is (in logic or in join implementation).
          Here's explain of org.apache.calcite.test.JdbcTest#testWhereOr:

          EnumerableSort(sort0=[$0], sort1=[$1], dir0=[ASC], dir1=[ASC])
            EnumerableJoin(condition=[true], joinType=[inner])
              EnumerableCalc(expr#0..1=[{inputs}], value=[$t1])
                EnumerableTableScan(table=[[s, bools]])
              EnumerableCalc(expr#0..1=[{inputs}], expr#2=[CAST($t1):VARCHAR(1) CHARACTER SET "ISO-8859-1" COLLATE "ISO-8859-1$en_US$primary"], expr#3=['T'], expr#4=[=($t2, $t3)], value=[$t1], $condition=[$t4])
                EnumerableTableScan(table=[[s, bools]])

          2) "int_column='text'" fails at janino compilation. Not sure if we could do better job for error reporting.
          3) I think "unified diff format" or "side by side" one would be much better. It would be good if Quidem could output IDEA-compatible AsesrtionError (the one that has "open diff" hyperlink when you use regular assertEquals(a,b,message))

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - Julian Hyde , it is not yet ready I've reworked PR and identified a couple of new bugs: 1) "a or b" does not return (null; true) row somehow. <-- I believe this is a showstopper. I'm not sure where the bug is (in logic or in join implementation). Here's explain of org.apache.calcite.test.JdbcTest#testWhereOr: EnumerableSort(sort0=[$0], sort1=[$1], dir0=[ASC], dir1=[ASC]) EnumerableJoin(condition=[true], joinType=[inner]) EnumerableCalc(expr#0..1=[{inputs}], value=[$t1]) EnumerableTableScan(table=[[s, bools]]) EnumerableCalc(expr#0..1=[{inputs}], expr#2=[CAST($t1):VARCHAR(1) CHARACTER SET "ISO-8859-1" COLLATE "ISO-8859-1$en_US$primary"], expr#3=['T'], expr#4=[=($t2, $t3)], value=[$t1], $condition=[$t4]) EnumerableTableScan(table=[[s, bools]]) 2) "int_column='text'" fails at janino compilation. Not sure if we could do better job for error reporting. 3) I think "unified diff format" or "side by side" one would be much better. It would be good if Quidem could output IDEA-compatible AsesrtionError (the one that has "open diff" hyperlink when you use regular assertEquals(a,b,message) )
          Hide
          julianhyde Julian Hyde added a comment -

          Please log separate JIRA cases for 1, 2, and a Quidem issue https://github.com/julianhyde/quidem/issues for 3.

          1. I agree this is critical; but we should still check in a fix for 980.

          2. IIRC we allow WHERE deptno = '50' as indeed we allow WHERE hireDate = '2015-01-01', and the semantics are to convert from string to numeric/date/time at run time, optimizing to compile time if the string is constant. That implies that we need to give run-time errors if the strings do not have valid format.

          3. You could add a command-line flag for that behavior. By default, Quidem needs to be idempotent: if a Quidem test "fails" (i.e. does not behave as expected), it should generate a script such that if that script is run, it will "succeed". If a query produces different results than expected, the result should be the actual results. (If output is not ordered, Quidem tries to minimize the difference between the expected and actual.)

          Show
          julianhyde Julian Hyde added a comment - Please log separate JIRA cases for 1, 2, and a Quidem issue https://github.com/julianhyde/quidem/issues for 3. 1. I agree this is critical; but we should still check in a fix for 980. 2. IIRC we allow WHERE deptno = '50' as indeed we allow WHERE hireDate = '2015-01-01' , and the semantics are to convert from string to numeric/date/time at run time, optimizing to compile time if the string is constant. That implies that we need to give run-time errors if the strings do not have valid format. 3. You could add a command-line flag for that behavior. By default, Quidem needs to be idempotent: if a Quidem test "fails" (i.e. does not behave as expected), it should generate a script such that if that script is run, it will "succeed". If a query produces different results than expected, the result should be the actual results. (If output is not ordered, Quidem tries to minimize the difference between the expected and actual.)
          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - Fixed in https://git-wip-us.apache.org/repos/asf?p=calcite.git;a=commit;h=963ba1b1b3d2ab95989d8383e0a855c3ae5e24cb
          Hide
          vladimirsitnikov Vladimir Sitnikov added a comment -

          Re 3, you are missing the point
          Here's problematic code: https://github.com/apache/calcite/blob/963ba1b1b3d2ab95989d8383e0a855c3ae5e24cb/core/src/test/java/org/apache/calcite/test/JdbcTest.java#L4772-L4775

          It would be much better to use assertEquals("result for " + path + " differ", DiffTestCase.fileContents(inFile), DiffTestCase.fileContents(outFile));.

          It think something like https://github.com/apache/calcite/blob/4c7f5c20a04b4a4e736a16f801d8b5e6eded48cc/core/src/test/java/org/apache/calcite/test/DiffTestCase.java#L347-L359 would make .oq debuging much easier.

          Show
          vladimirsitnikov Vladimir Sitnikov added a comment - Re 3, you are missing the point Here's problematic code: https://github.com/apache/calcite/blob/963ba1b1b3d2ab95989d8383e0a855c3ae5e24cb/core/src/test/java/org/apache/calcite/test/JdbcTest.java#L4772-L4775 It would be much better to use assertEquals("result for " + path + " differ", DiffTestCase.fileContents(inFile), DiffTestCase.fileContents(outFile)); . It think something like https://github.com/apache/calcite/blob/4c7f5c20a04b4a4e736a16f801d8b5e6eded48cc/core/src/test/java/org/apache/calcite/test/DiffTestCase.java#L347-L359 would make .oq debuging much easier.
          Hide
          julianhyde Julian Hyde added a comment -

          Oh sure, go ahead and fix 3, or at least log a jira.

          Show
          julianhyde Julian Hyde added a comment - Oh sure, go ahead and fix 3, or at least log a jira.
          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.6.0 (2016-01-22).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.6.0 (2016-01-22).

            People

            • Assignee:
              vladimirsitnikov Vladimir Sitnikov
              Reporter:
              liyang.gmt8@gmail.com liyang
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development