Skip to content
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

fix(sql): breaking💥 fix coalesce function correctness #4490

Merged
merged 47 commits into from
May 20, 2024

Conversation

bluestreak01
Copy link
Member

@bluestreak01 bluestreak01 commented May 9, 2024

fixes #4478
fixes #4477
fixes #4505

Scope

This PR impacts all SQL functions that accept var-arg argument. More specifically impacted functions are:

  • COALESCE
  • IN
  • CASE
  • CAST(NULL AS...

Bind variable behaviour (general)

Bind variables are utilized by:

  • preparing SQL statement
  • setting bind variable value and type
  • execute SQL to retrieve result for the bind variable value

The prepared statement can be reused. Potentially allowing user to change bind variable type from one execution to the next.

Old behaviour

The affected functions use to accept bind variables and implicitly type them as STRING during the preparation stage. When user sets type of bind variable as string, the result would be predictable. However, if user sets bind variable type to something other than string - the result is generally undefined. You can get anything between incorrect result and an obscure error.

New behaviour

After SQL preparation stage the type of bind variables is UNDEFINED. The type becomes defined once bind variable is assigned a value. Some functions cannot allow that, because type of their arguments also influences the return type of the function itself. As a result these function no longer accept bind variables:

  • COALESCE
  • CASE

Additionally this function:

  • COALESCE

enforces the types of its arguments to be "compatible", e.g. to belong to the same group of types. The groups of types are:

  • STRING,VARCHAR,CHAR
  • DOUBLE, FLOAT, INT, LONG, SHORT, BYTE
  • BOOLEAN
  • UUID
  • LONG256
  • IPV4

arguments, which types belong to different groups will be rejected by `COALESCE

This function:

  • IN

will now narrow down the type of bind variables to those compatible with the type of its left argument. The types must belong to the same group, as described for COALESCE

@jerrinot
Copy link
Contributor

this should be marked as a breaking change💥

@bluestreak01 bluestreak01 changed the title fix(sql): fix coalesce function correctness fix(sql): breaking💥 fix coalesce function correctness May 14, 2024
@bluestreak01 bluestreak01 marked this pull request as ready for review May 16, 2024 11:32
ideoma
ideoma previously approved these changes May 20, 2024
@ideoma
Copy link
Collaborator

ideoma commented May 20, 2024

[PR Coverage check]

😍 pass : 771 / 872 (88.42%)

file detail

path covered line new line coverage
🔵 io/questdb/griffin/SqlException.java 0 3 00.00%
🔵 io/questdb/griffin/engine/functions/conditional/NullCaseFunction.java 5 44 11.36%
🔵 io/questdb/griffin/engine/functions/bind/BindVariableServiceImpl.java 2 5 40.00%
🔵 io/questdb/cutlass/pgwire/PGConnectionContext.java 68 85 80.00%
🔵 io/questdb/griffin/engine/functions/conditional/CoalesceFunctionFactory.java 44 52 84.62%
🔵 io/questdb/griffin/engine/functions/bool/InLongFunctionFactory.java 35 40 87.50%
🔵 io/questdb/std/Uuid.java 15 17 88.24%
🔵 io/questdb/std/Numbers.java 8 9 88.89%
🔵 io/questdb/griffin/engine/functions/bool/InTimestampTimestampFunctionFactory.java 62 69 89.86%
🔵 io/questdb/griffin/engine/functions/bool/InDoubleFunctionFactory.java 47 52 90.38%
🔵 io/questdb/griffin/engine/functions/bool/InUuidFunctionFactory.java 68 74 91.89%
🔵 io/questdb/griffin/engine/functions/bool/InVarcharFunctionFactory.java 37 39 94.87%
🔵 io/questdb/griffin/engine/functions/str/ConcatFunctionFactory.java 51 53 96.23%
🔵 io/questdb/griffin/engine/functions/conditional/CaseCommon.java 154 155 99.35%
🔵 io/questdb/griffin/engine/functions/conditional/CaseFunctionFactory.java 4 4 100.00%
🔵 io/questdb/griffin/engine/functions/bool/InTimestampStrFunctionFactory.java 2 2 100.00%
🔵 io/questdb/griffin/engine/functions/finance/LevelTwoPriceFunctionFactory.java 27 27 100.00%
🔵 io/questdb/griffin/engine/ExplainPlanFactory.java 3 3 100.00%
🔵 io/questdb/griffin/engine/functions/conditional/SwitchFunctionFactory.java 10 10 100.00%
🔵 io/questdb/griffin/engine/functions/rnd/RndUUIDCFunctionFactory.java 24 24 100.00%
🔵 io/questdb/griffin/engine/functions/bind/IndexedParameterLinkFunction.java 2 2 100.00%
🔵 io/questdb/griffin/engine/functions/bool/InStrFunctionFactory.java 36 36 100.00%
🔵 io/questdb/griffin/FunctionParser.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/bool/InSymbolFunctionFactory.java 14 14 100.00%
🔵 io/questdb/griffin/engine/functions/constants/Constants.java 3 3 100.00%
🔵 io/questdb/griffin/model/IntervalUtils.java 5 5 100.00%
🔵 io/questdb/griffin/engine/functions/bool/InCharFunctionFactory.java 2 2 100.00%
🔵 io/questdb/griffin/SqlExecutionContextImpl.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/catalogue/TypeOfFunctionFactory.java 31 31 100.00%
🔵 io/questdb/cairo/CairoEngine.java 1 1 100.00%
🔵 io/questdb/std/LongLongHashSet.java 5 5 100.00%
🔵 io/questdb/cutlass/pgwire/AbstractTypeContainer.java 4 4 100.00%

@bluestreak01 bluestreak01 merged commit f3fb000 into master May 20, 2024
24 checks passed
@bluestreak01 bluestreak01 deleted the vi_coalesce branch May 20, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants