[jira] [Commented] (CALCITE-5967) UnsupportedOperationException while implementing a call that requires a special collator
[ https://issues.apache.org/jira/browse/CALCITE-5967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17762296#comment-17762296 ] Ruben Q L commented on CALCITE-5967: Sure, I'll apply those suggestions before merging the patch. > UnsupportedOperationException while implementing a call that requires a > special collator > > > Key: CALCITE-5967 > URL: https://issues.apache.org/jira/browse/CALCITE-5967 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Ruben Q L >Assignee: Ruben Q L >Priority: Major > Labels: pull-request-available > Fix For: 1.36.0 > > > Regression introduced by a minor change within CALCITE-5914, detected while > testing a downstream project with the latest Calcite main. > CALCITE-5914 (see > [2a96512c|https://github.com/apache/calcite/commit/2a96512c352bda4a5d9c0c80730f5c115ac363d6]) > introduced this apparently innocuous change in > {{RexImpTable#AbstractRexCallImplementor#unboxIfNecessary}}: > {code} > // old > argValueList.stream() > .map(AbstractRexCallImplementor::unboxExpression) > .collect(Collectors.toList()); > => > // new > Util.transform(argValueList, > AbstractRexCallImplementor::unboxExpression); > {code} > Both expressions seem equivalent, however there is a subtle difference: the > old one returns an {{ArrayList}} (where we can add new elements); whereas the > new one returns a {{TransformingList}} that extends {{AbstractList}} and that > does not support {{List#add}}. > After calling {{unboxIfNecessary}}, we might need to modify the argument list > if we need a special collator to perform the operation: > {code} > private ParameterExpression genValueStatement(...) { > ... > optimizedArgValueList = unboxIfNecessary(optimizedArgValueList); > final Expression callValue = > implementSafe(translator, call, optimizedArgValueList); > ... > } > @Override Expression implementSafe(...) { > ... > final Expression fieldComparator = > generateCollatorExpression(relDataType0.getCollation()); > if (fieldComparator != null) { > argValueList.add(fieldComparator); // <--- > UnsupportedOperationException! > } > ... > {code} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (CALCITE-5967) UnsupportedOperationException while implementing a call that requires a special collator
[ https://issues.apache.org/jira/browse/CALCITE-5967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17762213#comment-17762213 ] Julian Hyde commented on CALCITE-5967: -- Only one request. Improve the documentation of the test. As it stands it is very difficult to see what is being tested, i.e. what is the 'hard thing' that makes this test different from other tests. In tests, code doesn't always speak for itself. In {{{}RexImpTable{}}}, can you use {{{}FlatLists.append(argValueList, fieldComparator){}}}. Functional is better. > UnsupportedOperationException while implementing a call that requires a > special collator > > > Key: CALCITE-5967 > URL: https://issues.apache.org/jira/browse/CALCITE-5967 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Ruben Q L >Assignee: Ruben Q L >Priority: Major > Labels: pull-request-available > Fix For: 1.36.0 > > > Regression introduced by a minor change within CALCITE-5914, detected while > testing a downstream project with the latest Calcite main. > CALCITE-5914 (see > [2a96512c|https://github.com/apache/calcite/commit/2a96512c352bda4a5d9c0c80730f5c115ac363d6]) > introduced this apparently innocuous change in > {{RexImpTable#AbstractRexCallImplementor#unboxIfNecessary}}: > {code} > // old > argValueList.stream() > .map(AbstractRexCallImplementor::unboxExpression) > .collect(Collectors.toList()); > => > // new > Util.transform(argValueList, > AbstractRexCallImplementor::unboxExpression); > {code} > Both expressions seem equivalent, however there is a subtle difference: the > old one returns an {{ArrayList}} (where we can add new elements); whereas the > new one returns a {{TransformingList}} that extends {{AbstractList}} and that > does not support {{List#add}}. > After calling {{unboxIfNecessary}}, we might need to modify the argument list > if we need a special collator to perform the operation: > {code} > private ParameterExpression genValueStatement(...) { > ... > optimizedArgValueList = unboxIfNecessary(optimizedArgValueList); > final Expression callValue = > implementSafe(translator, call, optimizedArgValueList); > ... > } > @Override Expression implementSafe(...) { > ... > final Expression fieldComparator = > generateCollatorExpression(relDataType0.getCollation()); > if (fieldComparator != null) { > argValueList.add(fieldComparator); // <--- > UnsupportedOperationException! > } > ... > {code} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (CALCITE-5967) UnsupportedOperationException while implementing a call that requires a special collator
[ https://issues.apache.org/jira/browse/CALCITE-5967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17761863#comment-17761863 ] Ruben Q L commented on CALCITE-5967: [~julianhyde] do you have any other remark? Otherwise I'll move forward and merge the patch into main. > UnsupportedOperationException while implementing a call that requires a > special collator > > > Key: CALCITE-5967 > URL: https://issues.apache.org/jira/browse/CALCITE-5967 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Ruben Q L >Assignee: Ruben Q L >Priority: Major > Labels: pull-request-available > Fix For: 1.36.0 > > > Regression introduced by a minor change within CALCITE-5914, detected while > testing a downstream project with the latest Calcite main. > CALCITE-5914 (see > [2a96512c|https://github.com/apache/calcite/commit/2a96512c352bda4a5d9c0c80730f5c115ac363d6]) > introduced this apparently innocuous change in > {{RexImpTable#AbstractRexCallImplementor#unboxIfNecessary}}: > {code} > // old > argValueList.stream() > .map(AbstractRexCallImplementor::unboxExpression) > .collect(Collectors.toList()); > => > // new > Util.transform(argValueList, > AbstractRexCallImplementor::unboxExpression); > {code} > Both expressions seem equivalent, however there is a subtle difference: the > old one returns an {{ArrayList}} (where we can add new elements); whereas the > new one returns a {{TransformingList}} that extends {{AbstractList}} and that > does not support {{List#add}}. > After calling {{unboxIfNecessary}}, we might need to modify the argument list > if we need a special collator to perform the operation: > {code} > private ParameterExpression genValueStatement(...) { > ... > optimizedArgValueList = unboxIfNecessary(optimizedArgValueList); > final Expression callValue = > implementSafe(translator, call, optimizedArgValueList); > ... > } > @Override Expression implementSafe(...) { > ... > final Expression fieldComparator = > generateCollatorExpression(relDataType0.getCollation()); > if (fieldComparator != null) { > argValueList.add(fieldComparator); // <--- > UnsupportedOperationException! > } > ... > {code} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (CALCITE-5967) UnsupportedOperationException while implementing a call that requires a special collator
[ https://issues.apache.org/jira/browse/CALCITE-5967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17760270#comment-17760270 ] Ruben Q L commented on CALCITE-5967: Thanks for the feedback [~julianhyde], no apologies needed, as you say this was a hole in our test suite. I have adjusted the PR to fix the issue following your suggestion. I'm not sure if I can produce a test in SQL, I must admit that using strings with different collations is probably a rare feature, not sure if anybody else (besides my project and a few tests in Calcite) is using it. The problem was that, so far, all the tests in {{EnumerableStringComparisonTest}} in this regard focused on Sort (or sort-based operations like MergeJoin or MergeUnion) and we were lacking a test with a comparison operator call in e.g. a filter. > UnsupportedOperationException while implementing a call that requires a > special collator > > > Key: CALCITE-5967 > URL: https://issues.apache.org/jira/browse/CALCITE-5967 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Ruben Q L >Assignee: Ruben Q L >Priority: Major > Labels: pull-request-available > Fix For: 1.36.0 > > > Regression introduced by a minor change within CALCITE-5914, detected while > testing a downstream project with the latest Calcite main. > CALCITE-5914 (see > [2a96512c|https://github.com/apache/calcite/commit/2a96512c352bda4a5d9c0c80730f5c115ac363d6]) > introduced this apparently innocuous change in > {{RexImpTable#AbstractRexCallImplementor#unboxIfNecessary}}: > {code} > // old > argValueList.stream() > .map(AbstractRexCallImplementor::unboxExpression) > .collect(Collectors.toList()); > => > // new > Util.transform(argValueList, > AbstractRexCallImplementor::unboxExpression); > {code} > Both expressions seem equivalent, however there is a subtle difference: the > old one returns an {{ArrayList}} (where we can add new elements); whereas the > new one returns a {{TransformingList}} that extends {{AbstractList}} and that > does not support {{List#add}}. > After calling {{unboxIfNecessary}}, we might need to modify the argument list > if we need a special collator to perform the operation: > {code} > private ParameterExpression genValueStatement(...) { > ... > optimizedArgValueList = unboxIfNecessary(optimizedArgValueList); > final Expression callValue = > implementSafe(translator, call, optimizedArgValueList); > ... > } > @Override Expression implementSafe(...) { > ... > final Expression fieldComparator = > generateCollatorExpression(relDataType0.getCollation()); > if (fieldComparator != null) { > argValueList.add(fieldComparator); // <--- > UnsupportedOperationException! > } > ... > {code} -- This message was sent by Atlassian Jira (v8.20.10#820010)
[jira] [Commented] (CALCITE-5967) UnsupportedOperationException while implementing a call that requires a special collator
[ https://issues.apache.org/jira/browse/CALCITE-5967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17760074#comment-17760074 ] Julian Hyde commented on CALCITE-5967: -- Sorry I broke this. A better fix - the right fix - would be for the calling code to stop assuming that the list is mutable. That I didn't find the breakage speaks to a hole in our test suite. Is there a SQL-level test that would have found this? Your unit test is rather contrived and frankly I don't understand it. > UnsupportedOperationException while implementing a call that requires a > special collator > > > Key: CALCITE-5967 > URL: https://issues.apache.org/jira/browse/CALCITE-5967 > Project: Calcite > Issue Type: Bug > Components: core >Reporter: Ruben Q L >Assignee: Ruben Q L >Priority: Major > Labels: pull-request-available > Fix For: 1.36.0 > > > Regression introduced by a minor change within CALCITE-5914, detected while > testing a downstream project with the latest Calcite main. > CALCITE-5914 (see > [2a96512c|https://github.com/apache/calcite/commit/2a96512c352bda4a5d9c0c80730f5c115ac363d6]) > introduced this apparently innocuous change in > {{RexImpTable#AbstractRexCallImplementor#unboxIfNecessary}}: > {code} > // old > argValueList.stream() > .map(AbstractRexCallImplementor::unboxExpression) > .collect(Collectors.toList()); > => > // new > Util.transform(argValueList, > AbstractRexCallImplementor::unboxExpression); > {code} > Both expressions seem equivalent, however there is a subtle difference: the > old one returns an {{ArrayList}} (where we can add new elements); whereas the > new one returns a {{TransformingList}} that extends {{AbstractList}} and that > does not support {{List#add}}. > After calling {{unboxIfNecessary}}, we might need to modify the argument list > if we need a special collator to perform the operation: > {code} > private ParameterExpression genValueStatement(...) { > ... > optimizedArgValueList = unboxIfNecessary(optimizedArgValueList); > final Expression callValue = > implementSafe(translator, call, optimizedArgValueList); > ... > } > @Override Expression implementSafe(...) { > ... > final Expression fieldComparator = > generateCollatorExpression(relDataType0.getCollation()); > if (fieldComparator != null) { > argValueList.add(fieldComparator); // <--- > UnsupportedOperationException! > } > ... > {code} -- This message was sent by Atlassian Jira (v8.20.10#820010)