[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9143: IGNITE-14808 RIGHT|FULL Join operations are lost nulls sort ordering.

[GitBox <gi...@apache.org>]

alex-plekhanov commented on a change in pull request #9143:
URL: https://github.com/apache/ignite/pull/9143#discussion_r690144708



##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/schema/SchemaHolderImpl.java
##########
@@ -247,8 +248,11 @@ private static Object affinityIdentity(CacheConfiguration<?, ?> ccfg) {
             boolean descending = idxDesc.descending(idxField);
             int fieldIdx = fieldDesc.fieldIndex();
 
-            RelFieldCollation collation = new RelFieldCollation(fieldIdx,
-                descending ? RelFieldCollation.Direction.DESCENDING : RelFieldCollation.Direction.ASCENDING);
+            RelFieldCollation collation = new RelFieldCollation(
+                fieldIdx,
+                descending ? RelFieldCollation.Direction.DESCENDING : RelFieldCollation.Direction.ASCENDING,
+                RelFieldCollation.NullDirection.FIRST

Review comment:
       We have `NULLS LAST` for `DESC` order. 
   
   Also test for this should be added. For example, I've tested it with `CalciteBasicSecondaryIndexIntegrationTest` class, added a new index `CITY DESC`, put some nulls in `CITY` field and checked query `SELECT * FROM Developer ORDER BY city DESC nulls first`: currently it returns index scan without additional sort node and returns wrong nulls order.
   
   Also, shouldn't we change our `defaultNullCollation` to `LOW` to fit our indexes? Currently, if the user creates an index and then uses `order by` in a query by indexed columns, by default index will not be used without nulls first/nulls last clause. This can be confusing for some customers, perhaps behavior for index and `ORDER BY` should be consistent. In the current H2 engine, we have nulls ordering policy similar to calcites `LOW`.

##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/metadata/IgniteMdCollation.java
##########
@@ -548,9 +548,9 @@ public int compare(List<RexLiteral> o1, List<RexLiteral> o2) {
             case FULL:
                 for (RelCollation collation : leftCollations) {
                     for (RelFieldCollation field : collation.getFieldCollations()) {
-                        if (!(RelFieldCollation.NullDirection.LAST == field.nullDirection)) {
+                        if (!(RelFieldCollation.NullDirection.LAST.nullComparison ==

Review comment:
       Looks like these class contains a lot of unused code, this method unused too (applicable only for enumerable convention, but is not used even for enumerable convention, instead methods from `RelMdCollation` used)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org