[CALCITE-7316] The POSITION function in SQLite is missing the FROM clause#4854
[CALCITE-7316] The POSITION function in SQLite is missing the FROM clause#4854macroguo-ghy wants to merge 2 commits intoapache:mainfrom
Conversation
ca0c597 to
434840d
Compare
434840d to
ae31f91
Compare
| + "FROM \"foodmart\".\"product\""; | ||
| sql(query).withSQLite().ok(expected); | ||
|
|
||
| final String query1 = "select position('C' IN 'ABCABC' FROM 4) from \"product\""; |
There was a problem hiding this comment.
Pull request overview
Fixes SQLite SQL generation for the 3-argument POSITION(... FROM ...) form so the start-position semantics are preserved during unparsing.
Changes:
- Update
SqliteSqlDialectto unparse 3-argPOSITIONinto aCASEexpression based onSUBSTR+INSTR. - Add rel-to-sql tests for SQLite covering both a match and a no-match case for
POSITION(... FROM ...).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/src/main/java/org/apache/calcite/sql/dialect/SqliteSqlDialect.java | Adds operand-count-sensitive POSITION unparsing and a helper to rewrite the 3-arg form for SQLite. |
| core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java | Adds SQLite rel-to-sql assertions for POSITION(... FROM ...) (hit + miss). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final SqlNode thenResult = | ||
| SqlStdOperatorTable.PLUS.createCall(pos, relativePosition, startAdjustment); | ||
| return new SqlCase(pos, null, SqlNodeList.of(whenCondition), | ||
| SqlNodeList.of(thenResult), SqlLiteral.createExactNumeric("0", pos)); |
There was a problem hiding this comment.
In the 3-arg POSITION rewrite, the CASE expression uses ELSE 0. If any operand is NULL, INSTR(...) (and therefore relativePosition) becomes NULL; the WHEN relativePosition > 0 condition then evaluates to NULL and the CASE falls through to ELSE 0, changing the result from NULL to 0. To preserve POSITION/INSTR null-propagation semantics, make the ELSE branch return relativePosition (so it yields 0 on “not found” and NULL on NULL inputs), or add an explicit WHEN relativePosition IS NULL THEN NULL branch.
| SqlNodeList.of(thenResult), SqlLiteral.createExactNumeric("0", pos)); | |
| SqlNodeList.of(thenResult), SqlNode.clone(relativePosition)); |
…ause - bug fix , add jira link
xiedeyantu
left a comment
There was a problem hiding this comment.
Thank you for taking the time to address the Jira issue I raised. I’ve left a few points open for discussion—perhaps we can continue the conversation there in the Jira ticket as well.
| public class SqliteSqlDialect extends SqlDialect { | ||
| // Use plain function nodes here so the SQLite POSITION rewrite does not | ||
| // re-enter dialect-specific operator handling during unparsing. | ||
| private static final SqlFunction INSTR = |
There was a problem hiding this comment.
Why do we need to add additional function definitions in the dialect?
| sql(query).withSQLite().ok(expected); | ||
|
|
||
| final String query1 = "select position('C' IN 'ABCABC' FROM 4) from \"product\""; | ||
| final String expected1 = |
There was a problem hiding this comment.
I’m not sure if we need to make such complex rewrites. Honestly, I haven’t even figured out how to approach the rewrite in the Jira issue.
There was a problem hiding this comment.
+1. Additionally, there is another issue: for the query select * from db.table where d=position(a IN b FROM c), if this expression is placed within the WHERE clause, we still need to enclose it in parentheses.
|



Jira Link
CALCITE-7316
Changes Proposed
This PR fixes SQLite SQL generation for the 3-argument
POSITIONform so that theFROMstart position is preserved instead of being dropped.Reproduction:
Before this change, the generated SQLite SQL lost the
FROM 4semantics.This change rewrites the SQLite form to an equivalent expression based on
SUBSTRandINSTR, and adds SQLite rel-to-sql tests covering both a hit and a miss case forPOSITION(... FROM ...).