Skip to content

Commit 7feb3f3

Browse files
author
Selina Song
committed
Address code review: extend hasSameField to compare all fields and simplify null direction handling
1 parent dff4522 commit 7feb3f3

File tree

2 files changed

+43
-26
lines changed

2 files changed

+43
-26
lines changed

core/src/main/java/org/opensearch/sql/calcite/rule/SortReverseOptimizationRule.java

Lines changed: 37 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -70,30 +70,48 @@ public void onMatch(RelOptRuleCall call) {
7070
}
7171

7272
private boolean hasSameFieldWithOppositeDirection(LogicalSort outerSort, LogicalSort innerSort) {
73-
if (outerSort.getCollation().getFieldCollations().isEmpty()
74-
|| innerSort.getCollation().getFieldCollations().isEmpty()) {
73+
var outerFields = outerSort.getCollation().getFieldCollations();
74+
var innerFields = innerSort.getCollation().getFieldCollations();
75+
76+
if (outerFields.isEmpty() || innerFields.isEmpty()) {
7577
LOG.debug("No field collations found");
7678
return false;
7779
}
7880

79-
var outerField = outerSort.getCollation().getFieldCollations().get(0);
80-
var innerField = innerSort.getCollation().getFieldCollations().get(0);
81-
82-
LOG.debug(
83-
"Outer field: index={}, direction={}",
84-
outerField.getFieldIndex(),
85-
outerField.getDirection());
86-
LOG.debug(
87-
"Inner field: index={}, direction={}",
88-
innerField.getFieldIndex(),
89-
innerField.getDirection());
90-
91-
boolean sameField = outerField.getFieldIndex() == innerField.getFieldIndex();
92-
boolean oppositeDirection = outerField.getDirection() != innerField.getDirection();
81+
// Must have same number of fields
82+
if (outerFields.size() != innerFields.size()) {
83+
LOG.debug(
84+
"Different number of sort fields: outer={}, inner={}",
85+
outerFields.size(),
86+
innerFields.size());
87+
return false;
88+
}
9389

94-
LOG.debug("Same field: {}, Opposite direction: {}", sameField, oppositeDirection);
90+
// Check all fields have same index but opposite directions
91+
for (int i = 0; i < outerFields.size(); i++) {
92+
var outerField = outerFields.get(i);
93+
var innerField = innerFields.get(i);
94+
95+
LOG.debug(
96+
"Field {}: outer(index={}, direction={}), inner(index={}, direction={})",
97+
i,
98+
outerField.getFieldIndex(),
99+
outerField.getDirection(),
100+
innerField.getFieldIndex(),
101+
innerField.getDirection());
102+
103+
if (outerField.getFieldIndex() != innerField.getFieldIndex()
104+
|| outerField.getDirection() == innerField.getDirection()) {
105+
LOG.debug(
106+
"Field {} mismatch: same index={}, opposite direction={}",
107+
i,
108+
outerField.getFieldIndex() == innerField.getFieldIndex(),
109+
outerField.getDirection() != innerField.getDirection());
110+
return false;
111+
}
112+
}
95113

96-
// Must be same field with opposite directions
97-
return sameField && oppositeDirection;
114+
LOG.debug("All fields match with opposite directions");
115+
return true;
98116
}
99117
}

core/src/main/java/org/opensearch/sql/calcite/utils/PlanUtils.java

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -411,13 +411,12 @@ public static RelCollation reverseCollation(RelCollation original) {
411411
RelFieldCollation.Direction reversedDirection = field.direction.reverse();
412412

413413
// Handle null direction properly - reverse it as well
414-
RelFieldCollation.NullDirection reversedNullDirection = field.nullDirection;
415-
if (reversedNullDirection == RelFieldCollation.NullDirection.FIRST) {
416-
reversedNullDirection = RelFieldCollation.NullDirection.LAST;
417-
} else if (reversedNullDirection == RelFieldCollation.NullDirection.LAST) {
418-
reversedNullDirection = RelFieldCollation.NullDirection.FIRST;
419-
}
420-
// UNSPECIFIED remains UNSPECIFIED
414+
RelFieldCollation.NullDirection reversedNullDirection =
415+
field.nullDirection == RelFieldCollation.NullDirection.FIRST
416+
? RelFieldCollation.NullDirection.LAST
417+
: field.nullDirection == RelFieldCollation.NullDirection.LAST
418+
? RelFieldCollation.NullDirection.FIRST
419+
: field.nullDirection;
421420

422421
RelFieldCollation reversedField =
423422
new RelFieldCollation(field.getFieldIndex(), reversedDirection, reversedNullDirection);

0 commit comments

Comments
 (0)