Skip to content

Commit d1ffabd

Browse files
ahkcsahkcs
authored
bin command error message enhancement (#4690)
1 parent c6a5fb9 commit d1ffabd

File tree

10 files changed

+342
-32
lines changed

10 files changed

+342
-32
lines changed

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -337,4 +337,68 @@ public Type getJavaClass(RelDataType type) {
337337
public static boolean isUserDefinedType(RelDataType type) {
338338
return type instanceof AbstractExprRelDataType<?>;
339339
}
340+
341+
/**
342+
* Checks if the RelDataType represents a numeric field. Supports both standard SQL numeric types
343+
* (INTEGER, BIGINT, SMALLINT, TINYINT, FLOAT, DOUBLE, DECIMAL, REAL) and OpenSearch UDT numeric
344+
* types.
345+
*
346+
* @param fieldType the RelDataType to check
347+
* @return true if the type is numeric, false otherwise
348+
*/
349+
public static boolean isNumericType(RelDataType fieldType) {
350+
// Check standard SQL numeric types
351+
SqlTypeName sqlType = fieldType.getSqlTypeName();
352+
if (sqlType == SqlTypeName.INTEGER
353+
|| sqlType == SqlTypeName.BIGINT
354+
|| sqlType == SqlTypeName.SMALLINT
355+
|| sqlType == SqlTypeName.TINYINT
356+
|| sqlType == SqlTypeName.FLOAT
357+
|| sqlType == SqlTypeName.DOUBLE
358+
|| sqlType == SqlTypeName.DECIMAL
359+
|| sqlType == SqlTypeName.REAL) {
360+
return true;
361+
}
362+
363+
// Check for OpenSearch UDT numeric types
364+
if (isUserDefinedType(fieldType)) {
365+
AbstractExprRelDataType<?> exprType = (AbstractExprRelDataType<?>) fieldType;
366+
ExprType udtType = exprType.getExprType();
367+
return ExprCoreType.numberTypes().contains(udtType);
368+
}
369+
370+
return false;
371+
}
372+
373+
/**
374+
* Checks if the RelDataType represents a time-based field (timestamp, date, or time). Supports
375+
* both standard SQL time types (including TIMESTAMP, TIMESTAMP_WITH_LOCAL_TIME_ZONE, DATE, TIME,
376+
* and their timezone variants) and OpenSearch UDT time types.
377+
*
378+
* @param fieldType the RelDataType to check
379+
* @return true if the type is time-based, false otherwise
380+
*/
381+
public static boolean isTimeBasedType(RelDataType fieldType) {
382+
// Check standard SQL time types
383+
SqlTypeName sqlType = fieldType.getSqlTypeName();
384+
if (sqlType == SqlTypeName.TIMESTAMP
385+
|| sqlType == SqlTypeName.TIMESTAMP_WITH_LOCAL_TIME_ZONE
386+
|| sqlType == SqlTypeName.DATE
387+
|| sqlType == SqlTypeName.TIME
388+
|| sqlType == SqlTypeName.TIME_WITH_LOCAL_TIME_ZONE) {
389+
return true;
390+
}
391+
392+
// Check for OpenSearch UDT types (EXPR_TIMESTAMP mapped to VARCHAR)
393+
if (isUserDefinedType(fieldType)) {
394+
AbstractExprRelDataType<?> exprType = (AbstractExprRelDataType<?>) fieldType;
395+
ExprType udtType = exprType.getExprType();
396+
return udtType == ExprCoreType.TIMESTAMP
397+
|| udtType == ExprCoreType.DATE
398+
|| udtType == ExprCoreType.TIME;
399+
}
400+
401+
// Fallback check if type string contains EXPR_TIMESTAMP
402+
return fieldType.toString().contains("EXPR_TIMESTAMP");
403+
}
340404
}

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

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,11 @@
66
package org.opensearch.sql.calcite.utils.binning;
77

88
import java.util.List;
9-
import org.apache.calcite.rel.type.RelDataType;
10-
import org.apache.calcite.sql.type.SqlTypeName;
119
import org.opensearch.sql.ast.expression.Field;
1210
import org.opensearch.sql.ast.tree.Bin;
1311
import org.opensearch.sql.calcite.CalcitePlanContext;
14-
import org.opensearch.sql.calcite.type.AbstractExprRelDataType;
15-
import org.opensearch.sql.data.type.ExprCoreType;
16-
import org.opensearch.sql.data.type.ExprType;
1712

18-
/** Utility class for field validation and type checking in bin operations. */
13+
/** Utility class for bin-specific field operations. */
1914
public class BinFieldValidator {
2015

2116
/** Extracts the field name from a Bin node. */
@@ -37,26 +32,4 @@ public static void validateFieldExists(String fieldName, CalcitePlanContext cont
3732
"Field '%s' not found in dataset. Available fields: %s", fieldName, availableFields));
3833
}
3934
}
40-
41-
/** Checks if the field type is time-based. */
42-
public static boolean isTimeBasedField(RelDataType fieldType) {
43-
// Check standard SQL time types
44-
SqlTypeName sqlType = fieldType.getSqlTypeName();
45-
if (sqlType == SqlTypeName.TIMESTAMP
46-
|| sqlType == SqlTypeName.TIMESTAMP_WITH_LOCAL_TIME_ZONE
47-
|| sqlType == SqlTypeName.DATE) {
48-
return true;
49-
}
50-
51-
// Check for OpenSearch UDT types (EXPR_TIMESTAMP mapped to VARCHAR)
52-
if (fieldType instanceof AbstractExprRelDataType<?> exprType) {
53-
ExprType udtType = exprType.getExprType();
54-
return udtType == ExprCoreType.TIMESTAMP
55-
|| udtType == ExprCoreType.DATE
56-
|| udtType == ExprCoreType.TIME;
57-
}
58-
59-
// Check if type string contains EXPR_TIMESTAMP
60-
return fieldType.toString().contains("EXPR_TIMESTAMP");
61-
}
6235
}

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.calcite.utils.binning;
7+
8+
import lombok.Getter;
9+
import org.apache.calcite.rel.type.RelDataType;
10+
import org.apache.calcite.rex.RexNode;
11+
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
12+
import org.opensearch.sql.exception.SemanticCheckException;
13+
14+
/**
15+
* Represents a validated field that supports binning operations. The existence of this class
16+
* guarantees that validation has been run - the field is either numeric or time-based.
17+
*
18+
* <p>This design encodes validation in the type system, preventing downstream code from forgetting
19+
* to validate or running validation multiple times.
20+
*/
21+
@Getter
22+
public class BinnableField {
23+
private final RexNode fieldExpr;
24+
private final RelDataType fieldType;
25+
private final String fieldName;
26+
private final boolean isTimeBased;
27+
private final boolean isNumeric;
28+
29+
/**
30+
* Creates a validated BinnableField. Throws SemanticCheckException if the field is neither
31+
* numeric nor time-based.
32+
*
33+
* @param fieldExpr The Rex expression for the field
34+
* @param fieldType The relational data type of the field
35+
* @param fieldName The name of the field (for error messages)
36+
* @throws SemanticCheckException if the field is neither numeric nor time-based
37+
*/
38+
public BinnableField(RexNode fieldExpr, RelDataType fieldType, String fieldName) {
39+
this.fieldExpr = fieldExpr;
40+
this.fieldType = fieldType;
41+
this.fieldName = fieldName;
42+
43+
this.isTimeBased = OpenSearchTypeFactory.isTimeBasedType(fieldType);
44+
this.isNumeric = OpenSearchTypeFactory.isNumericType(fieldType);
45+
46+
// Validation: field must be either numeric or time-based
47+
if (!isNumeric && !isTimeBased) {
48+
throw new SemanticCheckException(
49+
String.format(
50+
"Cannot apply binning: field '%s' is non-numeric and not time-related, expected"
51+
+ " numeric or time-related type",
52+
fieldName));
53+
}
54+
}
55+
56+
/**
57+
* Returns true if this field requires numeric binning logic (not time-based binning).
58+
*
59+
* @return true if the field should use numeric binning, false if it should use time-based binning
60+
*/
61+
public boolean requiresNumericBinning() {
62+
return !isTimeBased;
63+
}
64+
}

‎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,7 +13,9 @@
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;
18+
import org.opensearch.sql.calcite.utils.binning.BinnableField;
1719
import org.opensearch.sql.expression.function.PPLBuiltinOperators;
1820

1921
/** Handler for bins-based (count) binning operations. */
@@ -25,6 +27,11 @@ public RexNode createExpression(
2527

2628
CountBin countBin = (CountBin) node;
2729

30+
// Create validated binnable field (validates that field is numeric or time-based)
31+
// Note: bins parameter works with both numeric and time-based fields
32+
String fieldName = BinFieldValidator.extractFieldName(node);
33+
BinnableField field = new BinnableField(fieldExpr, fieldExpr.getType(), fieldName);
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: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55

66
package org.opensearch.sql.calcite.utils.binning.handlers;
77

8-
import org.apache.calcite.rel.type.RelDataType;
98
import org.apache.calcite.rex.RexNode;
109
import org.apache.calcite.sql.fun.SqlStdOperatorTable;
1110
import org.opensearch.sql.ast.tree.Bin;
@@ -15,6 +14,7 @@
1514
import org.opensearch.sql.calcite.utils.BinTimeSpanUtils;
1615
import org.opensearch.sql.calcite.utils.binning.BinFieldValidator;
1716
import org.opensearch.sql.calcite.utils.binning.BinHandler;
17+
import org.opensearch.sql.calcite.utils.binning.BinnableField;
1818
import org.opensearch.sql.calcite.utils.binning.RangeFormatter;
1919

2020
/** Handler for default binning when no parameters are specified. */
@@ -25,11 +25,13 @@ public RexNode createExpression(
2525
Bin node, RexNode fieldExpr, CalcitePlanContext context, CalciteRexNodeVisitor visitor) {
2626

2727
DefaultBin defaultBin = (DefaultBin) node;
28-
RelDataType fieldType = fieldExpr.getType();
2928
String fieldName = BinFieldValidator.extractFieldName(node);
3029

30+
// Create validated binnable field (validates that field is numeric or time-based)
31+
BinnableField field = new BinnableField(fieldExpr, fieldExpr.getType(), fieldName);
32+
3133
// Use time-based binning for time fields
32-
if (BinFieldValidator.isTimeBasedField(fieldType)) {
34+
if (field.isTimeBased()) {
3335
BinFieldValidator.validateFieldExists(fieldName, context);
3436
return BinTimeSpanUtils.createBinTimeSpanExpression(fieldExpr, 1, "h", 0, context);
3537
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@
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;
20+
import org.opensearch.sql.calcite.utils.binning.BinnableField;
1921
import org.opensearch.sql.expression.function.PPLBuiltinOperators;
2022

2123
/** Handler for minspan-based binning operations. */
@@ -27,6 +29,16 @@ public RexNode createExpression(
2729

2830
MinSpanBin minSpanBin = (MinSpanBin) node;
2931

32+
// Create validated binnable field (validates that field is numeric or time-based)
33+
String fieldName = BinFieldValidator.extractFieldName(node);
34+
BinnableField field = new BinnableField(fieldExpr, fieldExpr.getType(), fieldName);
35+
36+
// Minspan binning requires numeric fields
37+
if (!field.requiresNumericBinning()) {
38+
throw new IllegalArgumentException(
39+
"Minspan binning is only supported for numeric fields, not time-based fields");
40+
}
41+
3042
RexNode minspanValue = visitor.analyze(minSpanBin.getMinspan(), context);
3143

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

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
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;
15+
import org.opensearch.sql.calcite.utils.binning.BinnableField;
1416
import org.opensearch.sql.expression.function.PPLBuiltinOperators;
1517

1618
/** Handler for range-based binning (start/end parameters only). */
@@ -22,6 +24,16 @@ public RexNode createExpression(
2224

2325
RangeBin rangeBin = (RangeBin) node;
2426

27+
// Create validated binnable field (validates that field is numeric or time-based)
28+
String fieldName = BinFieldValidator.extractFieldName(node);
29+
BinnableField field = new BinnableField(fieldExpr, fieldExpr.getType(), fieldName);
30+
31+
// Range binning requires numeric fields
32+
if (!field.requiresNumericBinning()) {
33+
throw new IllegalArgumentException(
34+
"Range binning (start/end) is only supported for numeric fields, not time-based fields");
35+
}
36+
2537
// Simple MIN/MAX calculation - cleaner than complex CASE expressions
2638
RexNode dataMin = context.relBuilder.min(fieldExpr).over().toRex();
2739
RexNode dataMax = context.relBuilder.max(fieldExpr).over().toRex();

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,12 @@ public RexNode createExpression(
2929

3030
SpanBin spanBin = (SpanBin) node;
3131

32+
// Create validated binnable field (validates that field is numeric or time-based)
33+
String fieldName = BinFieldValidator.extractFieldName(node);
34+
BinnableField field = new BinnableField(fieldExpr, fieldExpr.getType(), fieldName);
35+
3236
// Handle time-based fields
33-
if (BinFieldValidator.isTimeBasedField(fieldExpr.getType())) {
37+
if (field.isTimeBased()) {
3438
return handleTimeBasedSpan(spanBin, fieldExpr, context, visitor);
3539
}
3640

@@ -64,6 +68,7 @@ private RexNode handleTimeBasedSpan(
6468
private RexNode handleNumericOrLogSpan(
6569
SpanBin node, RexNode fieldExpr, CalcitePlanContext context, CalciteRexNodeVisitor visitor) {
6670

71+
// Field is already validated by createExpression - must be numeric since we're in this branch
6772
RexNode spanValue = visitor.analyze(node.getSpan(), context);
6873

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

0 commit comments

Comments
 (0)