Skip to content

Commit 9798be7

Browse files
ahkcsahkcs
committed
bin command error message enhancement
Signed-off-by: Kai Huang <[email protected]>
1 parent 0dd5949 commit 9798be7

File tree

8 files changed

+237
-0
lines changed

8 files changed

+237
-0
lines changed

‎core/src/main/java/org/opensearch/sql/calcite/utils/binning/BinFieldValidator.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
import org.opensearch.sql.calcite.type.AbstractExprRelDataType;
1515
import org.opensearch.sql.data.type.ExprCoreType;
1616
import org.opensearch.sql.data.type.ExprType;
17+
import org.opensearch.sql.exception.SemanticCheckException;
1718

1819
/** Utility class for field validation and type checking in bin operations. */
1920
public class BinFieldValidator {
@@ -59,4 +60,45 @@ public static boolean isTimeBasedField(RelDataType fieldType) {
5960
// Check if type string contains EXPR_TIMESTAMP
6061
return fieldType.toString().contains("EXPR_TIMESTAMP");
6162
}
63+
64+
/**
65+
* Validates that the field type is numeric for numeric binning operations.
66+
*
67+
* @param fieldType the RelDataType of the field to validate
68+
* @param fieldName the name of the field being validated
69+
* @throws SemanticCheckException if the field is not numeric
70+
*/
71+
public static void validateNumericField(RelDataType fieldType, String fieldName) {
72+
if (!isNumericField(fieldType)) {
73+
throw new SemanticCheckException(
74+
String.format(
75+
"Cannot apply binning: field '%s' is non-numeric and not time-related, expected"
76+
+ " numeric or time-related type",
77+
fieldName));
78+
}
79+
}
80+
81+
/** Checks if the field type is numeric. */
82+
private static boolean isNumericField(RelDataType fieldType) {
83+
// Check standard SQL numeric types
84+
SqlTypeName sqlType = fieldType.getSqlTypeName();
85+
if (sqlType == SqlTypeName.INTEGER
86+
|| sqlType == SqlTypeName.BIGINT
87+
|| sqlType == SqlTypeName.SMALLINT
88+
|| sqlType == SqlTypeName.TINYINT
89+
|| sqlType == SqlTypeName.FLOAT
90+
|| sqlType == SqlTypeName.DOUBLE
91+
|| sqlType == SqlTypeName.DECIMAL
92+
|| sqlType == SqlTypeName.REAL) {
93+
return true;
94+
}
95+
96+
// Check for OpenSearch UDT numeric types
97+
if (fieldType instanceof AbstractExprRelDataType<?> exprType) {
98+
ExprType udtType = exprType.getExprType();
99+
return ExprCoreType.numberTypes().contains(udtType);
100+
}
101+
102+
return false;
103+
}
62104
}

‎core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.opensearch.sql.calcite.CalcitePlanContext;
1414
import org.opensearch.sql.calcite.CalciteRexNodeVisitor;
1515
import org.opensearch.sql.calcite.utils.binning.BinConstants;
16+
import org.opensearch.sql.calcite.utils.binning.BinFieldValidator;
1617
import org.opensearch.sql.calcite.utils.binning.BinHandler;
1718
import org.opensearch.sql.expression.function.PPLBuiltinOperators;
1819

@@ -25,6 +26,12 @@ public RexNode createExpression(
2526

2627
CountBin countBin = (CountBin) node;
2728

29+
// Validate that the field is numeric or time-based (bins works with both)
30+
String fieldName = BinFieldValidator.extractFieldName(node);
31+
if (!BinFieldValidator.isTimeBasedField(fieldExpr.getType())) {
32+
BinFieldValidator.validateNumericField(fieldExpr.getType(), fieldName);
33+
}
34+
2835
Integer requestedBins = countBin.getBins();
2936
if (requestedBins == null) {
3037
requestedBins = BinConstants.DEFAULT_BINS;

‎core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/DefaultBinHandler.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,9 @@ public RexNode createExpression(
3434
return BinTimeSpanUtils.createBinTimeSpanExpression(fieldExpr, 1, "h", 0, context);
3535
}
3636

37+
// Validate that the field is numeric for default numeric binning
38+
BinFieldValidator.validateNumericField(fieldType, fieldName);
39+
3740
// Use numeric binning for numeric fields
3841
return createNumericDefaultBinning(fieldExpr, context);
3942
}

‎core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/MinSpanBinHandler.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.opensearch.sql.ast.tree.MinSpanBin;
1616
import org.opensearch.sql.calcite.CalcitePlanContext;
1717
import org.opensearch.sql.calcite.CalciteRexNodeVisitor;
18+
import org.opensearch.sql.calcite.utils.binning.BinFieldValidator;
1819
import org.opensearch.sql.calcite.utils.binning.BinHandler;
1920
import org.opensearch.sql.expression.function.PPLBuiltinOperators;
2021

@@ -27,6 +28,10 @@ public RexNode createExpression(
2728

2829
MinSpanBin minSpanBin = (MinSpanBin) node;
2930

31+
// Validate that the field is numeric
32+
String fieldName = BinFieldValidator.extractFieldName(node);
33+
BinFieldValidator.validateNumericField(fieldExpr.getType(), fieldName);
34+
3035
RexNode minspanValue = visitor.analyze(minSpanBin.getMinspan(), context);
3136

3237
if (!minspanValue.isA(LITERAL)) {

‎core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/RangeBinHandler.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import org.opensearch.sql.ast.tree.RangeBin;
1111
import org.opensearch.sql.calcite.CalcitePlanContext;
1212
import org.opensearch.sql.calcite.CalciteRexNodeVisitor;
13+
import org.opensearch.sql.calcite.utils.binning.BinFieldValidator;
1314
import org.opensearch.sql.calcite.utils.binning.BinHandler;
1415
import org.opensearch.sql.expression.function.PPLBuiltinOperators;
1516

@@ -22,6 +23,10 @@ public RexNode createExpression(
2223

2324
RangeBin rangeBin = (RangeBin) node;
2425

26+
// Validate that the field is numeric
27+
String fieldName = BinFieldValidator.extractFieldName(node);
28+
BinFieldValidator.validateNumericField(fieldExpr.getType(), fieldName);
29+
2530
// Simple MIN/MAX calculation - cleaner than complex CASE expressions
2631
RexNode dataMin = context.relBuilder.min(fieldExpr).over().toRex();
2732
RexNode dataMax = context.relBuilder.max(fieldExpr).over().toRex();

‎core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/SpanBinHandler.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@ private RexNode handleTimeBasedSpan(
6464
private RexNode handleNumericOrLogSpan(
6565
SpanBin node, RexNode fieldExpr, CalcitePlanContext context, CalciteRexNodeVisitor visitor) {
6666

67+
// Validate that the field is numeric
68+
String fieldName = BinFieldValidator.extractFieldName(node);
69+
BinFieldValidator.validateNumericField(fieldExpr.getType(), fieldName);
70+
6771
RexNode spanValue = visitor.analyze(node.getSpan(), context);
6872

6973
if (!spanValue.isA(LITERAL)) {

‎integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteBinCommandIT.java‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package org.opensearch.sql.calcite.remote;
77

8+
import static org.junit.Assert.assertTrue;
89
import static org.opensearch.sql.legacy.TestsConstants.*;
910
import static org.opensearch.sql.util.MatcherUtils.rows;
1011
import static org.opensearch.sql.util.MatcherUtils.schema;
@@ -506,6 +507,145 @@ public void testBinWithNonExistentField() {
506507
errorMessage.contains("non_existent_field") || errorMessage.contains("not found"));
507508
}
508509

510+
@Test
511+
public void testBinWithMinspanOnNonNumericField() {
512+
// Test that bin command with minspan throws clear error for non-numeric field
513+
ResponseException exception =
514+
assertThrows(
515+
ResponseException.class,
516+
() -> {
517+
executeQuery(
518+
String.format(
519+
"source=%s | bin firstname minspan=10 | head 1", TEST_INDEX_ACCOUNT));
520+
});
521+
522+
// Get the full error message
523+
String errorMessage = exception.getMessage();
524+
525+
// Verify the error message is clear and specific
526+
String expectedMessage =
527+
"Cannot apply binning: field 'firstname' is non-numeric and not time-related, expected"
528+
+ " numeric or time-related type";
529+
assertTrue(
530+
"Error message should contain: '" + expectedMessage + "'",
531+
errorMessage.contains(expectedMessage));
532+
}
533+
534+
@Test
535+
public void testBinWithSpanOnNonNumericField() {
536+
// Test that bin command with span throws clear error for non-numeric field
537+
ResponseException exception =
538+
assertThrows(
539+
ResponseException.class,
540+
() -> {
541+
executeQuery(
542+
String.format("source=%s | bin lastname span=5 | head 1", TEST_INDEX_ACCOUNT));
543+
});
544+
545+
// Get the full error message
546+
String errorMessage = exception.getMessage();
547+
548+
// Verify the error message is clear and specific
549+
String expectedMessage =
550+
"Cannot apply binning: field 'lastname' is non-numeric and not time-related, expected"
551+
+ " numeric or time-related type";
552+
assertTrue(
553+
"Error message should contain: '" + expectedMessage + "'",
554+
errorMessage.contains(expectedMessage));
555+
}
556+
557+
@Test
558+
public void testBinWithBinsOnNonNumericField() {
559+
// Test that bin command with bins throws clear error for non-numeric field
560+
ResponseException exception =
561+
assertThrows(
562+
ResponseException.class,
563+
() -> {
564+
executeQuery(
565+
String.format("source=%s | bin state bins=10 | head 1", TEST_INDEX_ACCOUNT));
566+
});
567+
568+
// Get the full error message
569+
String errorMessage = exception.getMessage();
570+
571+
// Verify the error message is clear and specific
572+
String expectedMessage =
573+
"Cannot apply binning: field 'state' is non-numeric and not time-related, expected numeric"
574+
+ " or time-related type";
575+
assertTrue(
576+
"Error message should contain: '" + expectedMessage + "'",
577+
errorMessage.contains(expectedMessage));
578+
}
579+
580+
@Test
581+
public void testBinWithStartEndOnNonNumericField() {
582+
// Test that bin command with start/end throws clear error for non-numeric field
583+
ResponseException exception =
584+
assertThrows(
585+
ResponseException.class,
586+
() -> {
587+
executeQuery(
588+
String.format(
589+
"source=%s | bin city start=0 end=100 | head 1", TEST_INDEX_ACCOUNT));
590+
});
591+
592+
// Get the full error message
593+
String errorMessage = exception.getMessage();
594+
595+
// Verify the error message is clear and specific
596+
String expectedMessage =
597+
"Cannot apply binning: field 'city' is non-numeric and not time-related, expected numeric"
598+
+ " or time-related type";
599+
assertTrue(
600+
"Error message should contain: '" + expectedMessage + "'",
601+
errorMessage.contains(expectedMessage));
602+
}
603+
604+
@Test
605+
public void testBinDefaultOnNonNumericField() {
606+
// Test that default bin (no parameters) throws clear error for non-numeric field
607+
ResponseException exception =
608+
assertThrows(
609+
ResponseException.class,
610+
() -> {
611+
executeQuery(String.format("source=%s | bin email | head 1", TEST_INDEX_ACCOUNT));
612+
});
613+
614+
// Get the full error message
615+
String errorMessage = exception.getMessage();
616+
617+
// Verify the error message is clear and specific
618+
String expectedMessage =
619+
"Cannot apply binning: field 'email' is non-numeric and not time-related, expected numeric"
620+
+ " or time-related type";
621+
assertTrue(
622+
"Error message should contain: '" + expectedMessage + "'",
623+
errorMessage.contains(expectedMessage));
624+
}
625+
626+
@Test
627+
public void testBinLogSpanOnNonNumericField() {
628+
// Test that bin command with log span throws clear error for non-numeric field
629+
ResponseException exception =
630+
assertThrows(
631+
ResponseException.class,
632+
() -> {
633+
executeQuery(
634+
String.format("source=%s | bin gender span=log10 | head 1", TEST_INDEX_ACCOUNT));
635+
});
636+
637+
// Get the full error message
638+
String errorMessage = exception.getMessage();
639+
640+
// Verify the error message is clear and specific
641+
String expectedMessage =
642+
"Cannot apply binning: field 'gender' is non-numeric and not time-related, expected numeric"
643+
+ " or time-related type";
644+
assertTrue(
645+
"Error message should contain: '" + expectedMessage + "'",
646+
errorMessage.contains(expectedMessage));
647+
}
648+
509649
@Test
510650
public void testBinSpanWithStartEndNeverShrinkRange() throws IOException {
511651
JSONObject result =

‎ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBinTest.java‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import org.apache.calcite.rel.RelNode;
99
import org.apache.calcite.test.CalciteAssert;
1010
import org.junit.Test;
11+
import org.opensearch.sql.exception.SemanticCheckException;
1112

1213
public class CalcitePPLBinTest extends CalcitePPLAbstractTest {
1314

@@ -118,4 +119,34 @@ public void testBinWithAligntime() {
118119
+ " 3600 / 1) * 3600) `SYS_START`\n"
119120
+ "FROM `scott`.`products_temporal`");
120121
}
122+
123+
@Test(expected = SemanticCheckException.class)
124+
public void testBinWithMinspanOnNonNumericField() {
125+
String ppl = "source=EMP | bin ENAME minspan=10";
126+
getRelNode(ppl); // Should throw SemanticCheckException
127+
}
128+
129+
@Test(expected = SemanticCheckException.class)
130+
public void testBinWithSpanOnNonNumericField() {
131+
String ppl = "source=EMP | bin JOB span=5";
132+
getRelNode(ppl); // Should throw SemanticCheckException
133+
}
134+
135+
@Test(expected = SemanticCheckException.class)
136+
public void testBinWithBinsOnNonNumericField() {
137+
String ppl = "source=EMP | bin ENAME bins=10";
138+
getRelNode(ppl); // Should throw SemanticCheckException
139+
}
140+
141+
@Test(expected = SemanticCheckException.class)
142+
public void testBinWithStartEndOnNonNumericField() {
143+
String ppl = "source=EMP | bin JOB start=1 end=10";
144+
getRelNode(ppl); // Should throw SemanticCheckException
145+
}
146+
147+
@Test(expected = SemanticCheckException.class)
148+
public void testBinDefaultOnNonNumericField() {
149+
String ppl = "source=EMP | bin ENAME";
150+
getRelNode(ppl); // Should throw SemanticCheckException
151+
}
121152
}

0 commit comments

Comments
 (0)