[SQL] Fix bug implementing incorrect array cast #5346
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the SQL compiler related to incorrect array casting behavior. The fix removes problematic special-case handling for empty arrays with DBSPTypeAny elements and improves cast expression handling in the Rust code generation backend.
Key Changes
- Removed early-return optimization for empty array casts that was causing type inference failures
- Enhanced compact mode cast expression generation to properly wrap casts with parentheses
- Added regression test to prevent recurrence of the array cast bug
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sql-to-dbsp-compiler/SQL-compiler/src/test/java/org/dbsp/sqlCompiler/compiler/sql/simple/Regression1Tests.java | Added regression test issue5345() that verifies the array cast bug is fixed and removed unused @Ignore import |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/visitors/inner/ExpandCasts.java | Removed incorrect special-case handling for empty arrays with DBSPTypeAny elements during cast expansion |
| sql-to-dbsp-compiler/SQL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/rust/ToRustInnerVisitor.java | Added proper parenthesized cast syntax generation for compact mode instead of early return |
...QL-compiler/src/main/java/org/dbsp/sqlCompiler/compiler/backend/rust/ToRustInnerVisitor.java
Show resolved
Hide resolved
| if (this.compact) { | ||
| this.builder.append("((") | ||
| .append(expression.type) | ||
| .append(")") | ||
| .append(expression.source) |
There was a problem hiding this comment.
This doesn't seem like it produces valid rust. Is this compact format only for intermediate code / debugging?
There was a problem hiding this comment.
it is only used for generating dot files right now.
some operations cannot even be synthesized as rust, but they should produce usable debugging output before they are converted into operations that can be synthesized as rust
| DBSPTypeArray sourceVecType = sourceType.to(DBSPTypeArray.class); | ||
| // If the element type does not match, need to convert all elements | ||
| if (!type.getElementType().equals(sourceVecType.getElementType())) { | ||
| if (sourceVecType.getElementType().is(DBSPTypeAny.class)) { |
There was a problem hiding this comment.
the bug fix is here
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
blp
left a comment
blp
left a comment
There was a problem hiding this comment.
I like fixing bugs by deleting code.
Signed-off-by: Mihai Budiu <[email protected]>
mihaibudiu
force-pushed
the
issue5345
branch
from
92eeee1 to
fad4b21
Compare
|
Sorry to disappoint you @blp, I had to add some more code. |
|
This is all due to a Calcite bug https://issues.apache.org/jira/browse/CALCITE-7347 |
No description provided.