okumin commented on code in PR #5627:
URL: https://github.com/apache/hive/pull/5627#discussion_r1952626944


##########
ql/src/test/results/clientpositive/llap/materialized_view_partitioned.q.out:
##########
@@ -1154,11 +1154,11 @@ POSTHOOK: Input: default@partition_mv_3
 POSTHOOK: Input: default@partition_mv_3@key=238
 #### A masked pattern was here ####
 val_238_n      238
-val_238_n      238
 val_238        238
 val_238        238
 val_238        238
 val_238        238
+val_238_n      238

Review Comment:
   Although this doesn't violate the semantics, I didn't understand why this 
diff showed up



##########
iceberg/iceberg-handler/src/main/java/org/apache/iceberg/mr/hive/HiveIcebergStorageHandler.java:
##########
@@ -857,7 +858,12 @@ public DynamicPartitionCtx createDPContext(
           if (sortField.sourceId() == field.fieldId()) {
             customSortPositions.add(pos);
             customSortOrder.add(sortField.direction() == SortDirection.ASC ? 1 
: 0);
-            customSortNullOrder.add(sortField.nullOrder() == 
NullOrder.NULLS_FIRST ? 0 : 1);
+
+            NullOrdering nullOrdering = NullOrdering.NULLS_LAST;
+            if (sortField.nullOrder() == NullOrder.NULLS_FIRST) {
+              nullOrdering = NullOrdering.NULLS_FIRST;
+            }
+            customSortNullOrder.add(nullOrdering.getCode());

Review Comment:
   This seems to be OK as a conclusion, but I am leaving a note that came to my 
mind.
   
   We added a feature to configure the sort order of Iceberg tables. I wondered 
if we can keep the semantics of `WRITE LOCALLY ORDERED BY a`. 
   https://github.com/apache/hive/pull/5541/files#r1894857758
   
   The null ordering of sort order specs is required. So, our DDL specifies 
either NULLS_FIRST or NULLS_LAST, and then `createDPContext` just follows the 
definition. In other words, `sortField.nullOrder()` is non-null and this change 
never takes effect in the real world.
   
https://github.com/apache/iceberg/blob/8839c9bf1f1d8c9b718f9766302ff8a2018e515f/core/src/main/java/org/apache/iceberg/SortOrderParser.java#L159-L175



-- 
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: gitbox-unsubscr...@hive.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org

Reply via email to