-
Notifications
You must be signed in to change notification settings - Fork 2k
[Feature][SQL function] Enhance SQL Transform COALESCE functions to support type cast #9299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request enhances the SQL Transform functions (COALESCE and IFNULL) to support type casting, ensuring that when arguments differ in type the correct type conversion is applied. Key changes include:
- Adding test cases to verify type conversion behavior in COALESCE and IFNULL.
- Extending SystemFunction with new overloads that accept a target type, along with a type-mapper table for casting.
- Updating ZetaSQLFunction to determine and apply the target type for COALESCE/IFNULL calls and updating documentation accordingly.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
seatunnel-transforms-v2/src/test/java/org/apache/seatunnel/transform/sql/SQLTransformTest.java | Added tests for verifying COALESCE/IFNULL type conversion behavior. |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/functions/SystemFunction.java | Implements enhanced coalesce functions with explicit type conversion and introduces a type mapper. |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLFunction.java | Modifies function evaluation to use enhanced system functions with target type conversion. |
docs/zh/transform-v2/sql-functions.md | Updates documentation to reflect the new type conversion behavior. |
docs/en/transform-v2/sql-functions.md | Updates documentation to reflect the new type conversion behavior. |
@@ -202,6 +202,9 @@ public class ZetaSQLFunction { | |||
|
|||
private final List<ZetaUDF> udfList; | |||
|
|||
// Add instance variable to track the current function being processed | |||
private Function currentFunction = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider parameterizing the raw 'Function' type with appropriate generic types (e.g., Function<Expression, SeaTunnelDataType<?>>) to improve type safety and clarity.
private Function currentFunction = null; | |
private Function<Expression, SeaTunnelDataType<?>> currentFunction = null; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't change it like this, because I need the Function class of JSqlParser to be changed, and if I change it, it will become the Functional Interface of Java, and the two types are completely different.
SeaTunnelDataType<?> targetType = | ||
zetaSQLType.getExpressionType(currentFunction); | ||
return SystemFunction.coalesce(args, targetType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we should get targetType
from currentFunction
not args[0]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the first argument is NULL (which is common in COALESCE and IFNULL functions), getting the type information from args[0] is unreliable because NULL itself has no specific type.
|
||
if (firstArg != null) { | ||
Function<Object, SeaTunnelDataType<?>> typeMapper = | ||
TYPE_MAPPERS.get(firstArg.getClass()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can reuse ZetaSQLType::getExpressionType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is not null, it can be obtained directly from the parameter. If you want to maintain a high degree of consistency in the code, it is better to change it to ZetaSQLType::getExpressionType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can reuse
ZetaSQLType::getExpressionType
+1
@@ -202,6 +202,9 @@ public class ZetaSQLFunction { | |||
|
|||
private final List<ZetaUDF> udfList; | |||
|
|||
// Add instance variable to track the current function being processed | |||
private Function currentFunction = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need currentFunction
any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLType.java
Outdated
Show resolved
Hide resolved
if (parameters == null) { | ||
throw new TransformException( | ||
CommonErrorCodeDeprecated.UNSUPPORTED_OPERATION, | ||
"COALESCE function requires at least one parameter"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"COALESCE function requires at least one parameter"); | |
function.getName() + " function requires at least one parameter"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
} else { | ||
throw new TransformException( | ||
CommonErrorCodeDeprecated.UNSUPPORTED_OPERATION, | ||
String.format("Unsupported CAST to INTEGER: %s", v1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String.format("Unsupported CAST to INTEGER: %s", v1)); | |
String.format("Unsupported CAST %s to INTEGER", v1)); |
} else { | ||
throw new TransformException( | ||
CommonErrorCodeDeprecated.UNSUPPORTED_OPERATION, | ||
String.format("Unsupported CAST to LONG: %s", v1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -203,4 +235,29 @@ public static Object castAs(List<Object> args) { | |||
CommonErrorCodeDeprecated.UNSUPPORTED_OPERATION, | |||
String.format("Unsupported CAST AS type: %s", v2)); | |||
} | |||
|
|||
// TYPE-MAP-TABLE | |||
private static final Map<Class<?>, Function<Object, SeaTunnelDataType<?>>> TYPE_MAPPERS = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can delete it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if ci passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the SQL Transform COALESCE function to support type casting and updates related functions and documentation.
- Adds new test cases for COALESCE behavior with different data types and null parameters.
- Updates SystemFunction, ZetaSQLType, and ZetaSQLFunction to incorporate a target type for conversions.
- Revises the documentation (both Chinese and English) to clarify the behavior of COALESCE with type conversions.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
seatunnel-transforms-v2/src/test/java/org/apache/seatunnel/transform/sql/SQLTransformTest.java | Adds comprehensive tests for COALESCE behavior under various scenarios. |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/functions/SystemFunction.java | Updates coalesce and ifnull functions to support type casting logic. |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLType.java | Revises function type determination for COALESCE with a helper for expression extraction. |
seatunnel-transforms-v2/src/main/java/org/apache/seatunnel/transform/sql/zeta/ZetaSQLFunction.java | Adjusts function invocations to pass the expression to determine target type. |
docs/zh/transform-v2/sql-functions.md | Enhances documentation to explain type conversion behavior in COALESCE. |
docs/en/transform-v2/sql-functions.md | Updates documentation to include type conversion examples for the COALESCE function. |
Exception exception = | ||
Assertions.assertThrows( | ||
Exception.class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using a more specific exception type (e.g., TransformException) instead of the generic Exception when testing non-existent columns so that the test precisely captures the expected failure condition.
Exception exception = | |
Assertions.assertThrows( | |
Exception.class, | |
TransformException exception = | |
Assertions.assertThrows( | |
TransformException.class, |
Copilot uses AI. Check for mistakes.
case "INTEGER": | ||
return Integer.parseInt(v1.toString()); | ||
if (v1 instanceof String) { | ||
return Integer.parseInt(v1.toString()); | ||
} else if (v1 instanceof Number) { | ||
return ((Number) v1).intValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding comments to clarify the conversion logic in this section for 'INT' and 'LONG' types to improve code readability and facilitate future maintenance.
case "INTEGER": | |
return Integer.parseInt(v1.toString()); | |
if (v1 instanceof String) { | |
return Integer.parseInt(v1.toString()); | |
} else if (v1 instanceof Number) { | |
return ((Number) v1).intValue(); | |
case "INTEGER": | |
// Handle conversion to INT/INTEGER type | |
// If the input is a String, attempt to parse it as an Integer | |
if (v1 instanceof String) { | |
return Integer.parseInt(v1.toString()); | |
// If the input is a Number, extract its integer value | |
} else if (v1 instanceof Number) { | |
return ((Number) v1).intValue(); | |
// Throw an exception for unsupported types |
Copilot uses AI. Check for mistakes.
@ocean-zhc Try again and resolve conflicts |
…upport type cast
Purpose of this pull request
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note
.