[CALCITE-7374] NULLS LAST throws ClassCastException when sorting arrays #4745
Conversation
| // If the trait is a composite trait, we return the first one as a | ||
| // representative trait. If RelCompositeTrait contain [[a, b], [a]]. | ||
| // Selects [a,b] as it implies [a]; preserves the strongest ordering. | ||
| return (T) ((RelCompositeTrait) trait).trait(0); |
There was a problem hiding this comment.
In the case of order by arr nulls last, the following error will be reported:
> java.sql.SQLException: Error while executing SQL "select * from
> (values
> (2, array[null, 3]),
> (3, array[3, 4]),
> (1, array[1, 2]),
> (4, array[4, 5]),
> (5, cast(null as integer array))) as t(id, arr)
> order by arr nulls last": class org.apache.calcite.plan.RelCompositeTrait cannot be cast to class org.apache.calcite.rel.RelCollation (org.apache.calcite.plan.RelCompositeTrait and org.apache.calcite.rel.RelCollation are in unnamed module of loader 'app')
> at org.apache.calcite.avatica.Helper.createException(Helper.java:56)
> at org.apache.calcite.avatica.Helper.createException(Helper.java:41)
> at org.apache.calcite.avatica.AvaticaStatement.executeInternal(AvaticaStatement.java:164)
> at org.apache.calcite.avatica.AvaticaStatement.executeQuery(AvaticaStatement.java:228)
> at net.hydromatic.quidem.Quidem.checkResult(Quidem.java:317)
> at net.hydromatic.quidem.Quidem.access$2600(Quidem.java:54)
> at net.hydromatic.quidem.Quidem$ContextImpl.checkResult(Quidem.java:1778)
> at net.hydromatic.quidem.Quidem$CheckResultCommand.execute(Quidem.java:985)
> at net.hydromatic.quidem.Quidem$CompositeCommand.execute(Quidem.java:1522)
> at net.hydromatic.quidem.Quidem.execute(Quidem.java:204)
> at org.apache.calcite.test.QuidemTest.checkRun(QuidemTest.java:350)
> at org.apache.calcite.test.QuidemTest.test(QuidemTest.java:532)
> at org.apache.calcite.test.CoreQuidemTest2.main(CoreQuidemTest2.java:38)
This is another error caused by nulls last, which is fixed here as well.
| // If the trait is a composite trait, we return the first one as a | ||
| // representative trait. If RelCompositeTrait contain [[a, b], [a]]. | ||
| // Selects [a,b] as it implies [a]; preserves the strongest ordering. | ||
| return (T) ((RelCompositeTrait) trait).trait(0); |
There was a problem hiding this comment.
is the first one always the most general?
There was a problem hiding this comment.
I think so; its constructor has a special sorting logic.
private RelCompositeTrait(RelTraitDef traitDef, T[] traits) {
this.traitDef = traitDef;
this.traits = requireNonNull(traits, "traits");
//noinspection unchecked
assert Ordering.natural()
.isStrictlyOrdered(Arrays.asList((Comparable[]) traits))
: Arrays.toString(traits);
for (T trait : traits) {
assert trait.getTraitDef() == this.traitDef;
}
}
There was a problem hiding this comment.
I am not sure why "sorted lexicographically" is the same as "the strongest first", but I don't know anything about the Traits.
There was a problem hiding this comment.
You're right, that's not rigorous, but returning only one collation here is indeed a bit radical. I think the design of RelCompositeTrait hasn't been used very well. I couldn't think of a better way to change it.
There was a problem hiding this comment.
But you have to implement "satisfies".
I wonder whether checking all traits in a set would work - if any of them satisfies the requested collation.
Then you don't need to worry about which one is "the most general"
There was a problem hiding this comment.
I think I need to revisit the RelCompositeTrait handling to see if I need to add a new interface, as the current getCollation() seems insufficient.
| } else if (o1 instanceof Object[] && o2 instanceof Object[]) { | ||
| return -compareObjectArrays((Object[]) o1, (Object[]) o2); | ||
| } else { | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
Please add here a descriptive error message; if this is ever hit, it will make debugging easier.
|
the checker doesn't like your changes |
xiedeyantu
added
the
LGTM-will-merge-soon
xiedeyantu
force-pushed
the
CALCITE-7374
branch
from
11bbf8e to
f2d4765
Compare
|
3 participants
See CALCITE-7374