Skip to content

fix(core): fix internal errors due to bad implicit casting#6443

Merged
bluestreak01 merged 17 commits intomasterfrom
jh_coltype_castign
Nov 28, 2025
Merged

fix(core): fix internal errors due to bad implicit casting#6443
bluestreak01 merged 17 commits intomasterfrom
jh_coltype_castign

Conversation

@jerrinot
Copy link
Contributor

@jerrinot jerrinot commented Nov 24, 2025 •
edited
Loading

This change fixes internal errors caused by the database being too lenient with type checking.

Example no. 1 - default values for lead/lag window functions:

create table tab (ts TIMESTAMP, i long, d double) timestamp(ts);
insert into tab values (now(), 0, 0.0);
select lead(i, 1, d) over () from tab; -- note, using a `double` value as a default for `long` function

Example no. 2 - array access

CREATE TABLE tango AS (SELECT ARRAY[
   [ [ 1,  2,  3], [ 4,  5,  6], [ 7,  8,  9] ],
   [ [10, 11, 12], [13, 14, 15], [16, 17, 18] ],
   [ [19, 20, 21], [22, 23, 24], [25, 26, 27] ]
] arr from long_sequence(1));
SELECT arr[1, 3.0] subarr FROM tango;

Example no. 3 - rnd_double_array() SQL function:

select rnd_double_array(1, 0.0, 10) from long_sequence(5);

Before this change, all of the examples above would fail with an internal error. After the change, they fail gracefully and produce a sensible error message.

Implementation details:

I suspect the main contributor to these bugs was poor naming: ColumnType.isAssignableFrom(fromType, toType) creates the impression that if it returns true for some combination of TypeA and TypeB, then it's legal to invoke typeAFunction.getTypeB(). Without adding any cast function. However, this is not the contract of isAssignableFrom(). This method merely indicates if it's possible to convert typeA and typeB at all, even it might requires a casting function. To prevent this confusion I renamed it from isAssignableFrom() to isConvertibleFrom() + added JavaDoc to explain the meaning. + I replaced the obviously bad invocations of isAssignableFrom() with isBuiltInWideningCast() or isSameOrBuiltInWideningCast().

This PR also fixes another bug: ColumnType.isBuiltInWideningCast() indicated some types were convertible between each other without any casting function even that was not the case. Example: ShortFunction.getTimestamp() throws UnsupportedOperationException but isBuiltInWideningCast(short, timestamp) used to return true. This is no longer the case. There were 3 cases in total, where the function used to lie:

  1. from BYTE to CHAR, DATE and TIMESTAMP
  2. from SHORT to DATE and TIMESTAMP
  3. from CHAR to DATE and TIMESTAMP

All these cases are now fixed. Insert used to rely on these bugs - since they would propagate to isToSameOrWider() and the insert machinery would create a casting function. After removing these cases from isBuiltInWideningCast(), I had to re-add them to isImplicitParsingCast() where they belongs anyway. It is a widening cast, but it is not built-in to base functions.

There is one more change to isBuiltInWideningCast(): It used to indicate that STRING can be converted to numeric (which is the case), but it would not advertise that VARCHAR can be converted to numeric just as well. I suspect this could have led to creating unnecessary casting functions. So I fixed this too.

I added a test validating that if isBuiltInWideningCast() returns true for some pair of types then we won't get the UnsupportedOperationException when we try to use them as such. I also tried to add the inverse test - that if isBuiltInWideningCast() returns false then types cannot be converted without a casting function, but this is more complicated - for example BooleanFunction has a function implementation of getByte(), but isBuiltInWideningCast() does not advertise it. I assume it's because it's not merely widening, but an actual conversion which should always be explicit. There are other cases like this, so I decided to just print them, instead of failing the test. Here is a list of these pairs:

  INFO: isBuiltInWideningCast(BOOLEAN, BYTE) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, SHORT) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, CHAR) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, INT) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, LONG) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, DATE) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, TIMESTAMP) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, FLOAT) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, DOUBLE) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, STRING) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, SYMBOL) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(BOOLEAN, VARCHAR) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(CHAR, BYTE) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(CHAR, STRING) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(CHAR, VARCHAR) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(TIMESTAMP, DATE) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(STRING, SYMBOL) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(STRING, IPv4) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(STRING, VARCHAR) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(SYMBOL, CHAR) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(SYMBOL, INT) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(SYMBOL, TIMESTAMP) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(SYMBOL, STRING) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(SYMBOL, VARCHAR) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(LONG256, LONG) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(unknown, GEOHASH) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(UUID, LONG128) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(GEOHASH, unknown) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(LONG128, UUID) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(VARCHAR, STRING) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(VARCHAR, SYMBOL) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(VARCHAR, IPv4) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(unknown, DECIMAL) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(unknown, unknown) = false, but getter works without UnsupportedOperationException
  INFO: isBuiltInWideningCast(DECIMAL, unknown) = false, but getter works without UnsupportedOperationException

fixes #6429

Considerations for reviewers: The bugs caused by accidental usage of isAssignableFrom() are directly connected to function signatures with vararg. Since without vararg FunctionParser creates casting functions as necessary and the target function receives arguments already converted to the expected type. However, functions accepting varargs are on their own - ideally, they should create implicit casting functions consistently with FunctionParser. That is NOT the case with this current state of the PR! This means even the fixed function will NOT perform implicit casting unless the casting is builtin.

tandem: https://github.com/questdb/questdb-enterprise/pull/797

@jerrinot jerrinot added the Bug Incorrect or unexpected behavior label Nov 24, 2025
@coderabbitai
Copy link

coderabbitai bot commented Nov 24, 2025 •
edited
Loading

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR refactors the type conversion system by removing the isAssignableFrom method from ColumnType and introducing three new type-checking methods: isBuiltInWideningCast, isConvertibleFrom, and isSameOrBuiltInWideningCast. These changes propagate across compiler logic and function factories to use more precise type compatibility checks.

Changes

Cohort / File(s) Summary
Type System API Refactoring
core/src/main/java/io/questdb/cairo/ColumnType.java		 Removed isAssignableFrom public method. Added three new public methods: isBuiltInWideningCast (tag-based with explicit exceptions), isConvertibleFrom (broader conversion check), isSameOrBuiltInWideningCast (identity or widening). Enhanced isImplicitParsingCast to include CHAR→DATE/TIMESTAMP and BYTE/SHORT→DATE/TIMESTAMP conversions.
Compiler Type Checking
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java, core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java		 Updated type-conversion validation from isAssignableFrom to isConvertibleFrom for runtime constant coercion and INSERT/UPDATE/type-casting paths. Broadens allowed implicit conversions during INSERT AS SELECT, UPDATE, and function type compatibility checks.
Array & Conditional Function Factories
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java, core/src/main/java/io/questdb/griffin/engine/functions/conditional/SwitchFunctionFactory.java		 Tightened or adjusted index/key type validation. DoubleArrayAccessFunctionFactory changed to stricter isBuiltInWideningCast for LONG types; error message now displays human-readable type names. SwitchFunctionFactory changed from isAssignableFrom to isConvertibleFrom.
Random Sequence Function Factories
core/src/main/java/io/questdb/griffin/engine/functions/rnd/LongSequenceFunctionFactory.java, core/src/main/java/io/questdb/griffin/engine/functions/rnd/RndDoubleArrayFunctionFactory.java		 LongSequenceFunctionFactory: single-arg path uses isConvertibleFrom; multi-arg uses isSameOrBuiltInWideningCast. RndDoubleArrayFunctionFactory: replaced isAssignableFrom with isSameOrBuiltInWideningCast for integer argument validation, addressing UnsupportedOperationException when non-integer types are passed.
Window Function Factories (Lag/Lead)
core/src/main/java/io/questdb/griffin/engine/functions/window/Lag*.java, core/src/main/java/io/questdb/griffin/engine/functions/window/Lead*.java		 All eight window functions (LagDate, LagDouble, LagLong, LagTimestamp, LeadDate, LeadDouble, LeadLong, LeadTimestamp) updated from isAssignableFrom to isSameOrBuiltInWideningCast for default-value type validation.
Test Updates
core/src/test/java/io/questdb/test/cairo/ArrayTest.java		 Updated error expectations and test assertions to use new isConvertibleFrom API. Added new tests testBadSlicingTypeFailsGracefully() and testRndArrayBadTypes() (note: appears duplicated). Error expectation for array access changed from type=1 to type=BOOLEAN.
New Test Infrastructure
core/src/test/java/io/questdb/test/cairo/ColumnTypeTest.java, core/src/test/java/io/questdb/test/griffin/engine/functions/rnd/LongSequenceTest.java		 ColumnTypeTest adds testIsBuiltInWideningCastContract() with helper methods to verify getter consistency. LongSequenceTest (new file) adds testBadArgumentTypeFailsGracefully() to validate error handling for invalid argument types.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Areas requiring extra attention:

  • ColumnType.java: Review the logic and exhaustiveness of isBuiltInWideningCast to ensure all type-pair exceptions are intentional and correctly handle the tag-based classification.
  • RndDoubleArrayFunctionFactory.java: Verify that the shift from isAssignableFrom to isSameOrBuiltInWideningCast resolves the linked UnsupportedOperationException (issue #6429) and doesn't introduce regressions when double arguments are passed.
  • SqlCompilerImpl.java: Audit all isConvertibleFrom calls across INSERT/UPDATE/type-casting paths to ensure the broader conversion set doesn't allow unsafe or unintended implicit casts.
  • Window function factories: Confirm that isSameOrBuiltInWideningCast replacement for default-value validation is semantically correct across eight functions and doesn't reject previously-valid default types.
  • Test duplication in ArrayTest.java: Investigate why testBadSlicingTypeFailsGracefully() and testRndArrayBadTypes() appear twice in the summary and ensure the final code doesn't contain duplicates.

Suggested reviewers

  • bluestreak01
  • puzpuzpuz
  • kafka1991

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'fix(core): fix internal errors due to bad implicit casting' clearly summarizes the main objective of the changeset—addressing internal errors caused by overly lenient type checking in implicit casting.
Linked Issues check ✅ Passed The changeset directly addresses issue #6429 by fixing RndDoubleArrayFunctionFactory's type validation and broader casting logic. Type-checking methods were corrected to prevent UnsupportedOperationException and enable graceful error handling.
Out of Scope Changes check ✅ Passed All code changes are focused on fixing type-checking and implicit casting bugs in ColumnType, function factories, SQL compilation, and related tests. No unrelated modifications to other systems or features were introduced.
Description check ✅ Passed The PR description clearly relates to the changeset: it explains the motivation for renaming isAssignableFrom to isConvertibleFrom, identifies specific bugs with implicit casting, provides concrete SQL examples that demonstrate the issues, and details the implementation approach taken across multiple files.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jerrinot
casting of these types is allowed.
e20d14a 
previously, isToSameOrWider() would return true because `isBuiltInWideningCast()` accidentally returned true too. even there is no builtin casting between these types.
@jerrinot
Copy link
Contributor Author

jerrinot commented Nov 24, 2025

@coderabbitai - please review

@coderabbitai
Copy link

coderabbitai bot commented Nov 24, 2025

@jerrinot: I'll review the changes in this PR.

🧠 Learnings used 
Learnt from: mtopolnik
Repo: questdb/questdb PR: 5997
File: core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java:127-131
Timestamp: 2025-07-31T08:21:17.390Z
Learning: In DoubleArrayAccessFunctionFactory.java, overflow validation for array index casting from long to int is performed in the validateIndexArgs() method rather than at each individual cast site. The validateIndexArgs() method is called early in newInstance() to validate all index arguments, making subsequent casts safe.
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot]
coderabbitai bot reviewed Nov 24, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
core/src/main/java/io/questdb/griffin/engine/functions/conditional/SwitchFunctionFactory.java (1)

95-100: Keys are not being cast when convertible types require casting functions.

The code has an asymmetric type conversion pattern:

  • Lines 86-104 (key loop, i = 1, 3, 5...): Uses isConvertibleFrom to validate key types but applies NO casting to keyFunc
  • Lines 106-119 (value loop, i = 2, 4, 6...): Applies CaseCommon.getCastFunction to convert values to returnType
  • Lines 121-125 (else branch): Also applies CaseCommon.getCastFunction

Since isConvertibleFrom may allow conversions requiring casting functions, keys should also receive casting via getCastFunction to ensure proper type conversion at runtime, similar to how values and the else branch are handled. Add a loop to cast key arguments (at indices 1, 3, 5...) to keyType after the value casting loop.

🧹 Nitpick comments (2)
core/src/test/java/io/questdb/test/cairo/ColumnTypeTest.java (1)

27-313: Robust contract test for isBuiltInWideningCast, minor range clarification

The new testIsBuiltInWideningCastContract plus callGetterForType / createTestFunction is a solid way to pin down the contract that “if isBuiltInWideningCast(from, to) is true, then the corresponding getter must not throw UnsupportedOperationException”, while still tolerating ImplicitCastException for value‑level failures and logging cases where getters work but aren’t advertised as widening casts. This should make future changes to ColumnType much safer.

One thing to double‑check: the iteration bounds

short allTypesLowerBoundInc = ColumnType.UNDEFINED + 1;
short allTypesUpperBoundEx = ColumnType.NULL;

mean you never exercise fromType == ColumnType.NULL or toType == ColumnType.NULL, even though isBuiltInWideningCast has special behavior for fromTag == NULL. If the intent is to include NULL in the contract check, it might be clearer to either extend the upper bound to include it or add it explicitly, and to document which tags are intentionally excluded via unsupportedTypes. Otherwise, the helpers and the rest of the test logic look correct and comprehensive.

core/src/test/java/io/questdb/test/griffin/engine/functions/rnd/LongSequenceTest.java (1)

35-38: Consider adding a positive test case for completeness.

The negative test case correctly validates graceful failure for incompatible argument types. Consider adding a complementary positive test case that verifies long_sequence works correctly with valid integer arguments (e.g., long_sequence(1, 5, 1)), ensuring the fix doesn't break valid usage.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b4a147e and e20d14a.

📒 Files selected for processing (18)
  • core/src/main/java/io/questdb/cairo/ColumnType.java (4 hunks)
  • core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java (5 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java (2 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/conditional/SwitchFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/rnd/LongSequenceFunctionFactory.java (2 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/rnd/RndDoubleArrayFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LagDateFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LagDoubleFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LagLongFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LagTimestampFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LeadDateFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LeadDoubleFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LeadLongFunctionFactory.java (1 hunks)
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LeadTimestampFunctionFactory.java (1 hunks)
  • core/src/test/java/io/questdb/test/cairo/ArrayTest.java (5 hunks)
  • core/src/test/java/io/questdb/test/cairo/ColumnTypeTest.java (3 hunks)
  • core/src/test/java/io/questdb/test/griffin/engine/functions/rnd/LongSequenceTest.java (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-31T08:21:17.390Z 
Learnt from: mtopolnik
Repo: questdb/questdb PR: 5997
File: core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java:127-131
Timestamp: 2025-07-31T08:21:17.390Z
Learning: In DoubleArrayAccessFunctionFactory.java, overflow validation for array index casting from long to int is performed in the validateIndexArgs() method rather than at each individual cast site. The validateIndexArgs() method is called early in newInstance() to validate all index arguments, making subsequent casts safe.

Applied to files:

  • core/src/main/java/io/questdb/griffin/engine/functions/rnd/RndDoubleArrayFunctionFactory.java
  • core/src/main/java/io/questdb/griffin/engine/functions/rnd/LongSequenceFunctionFactory.java
  • core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java
  • core/src/test/java/io/questdb/test/cairo/ArrayTest.java
  • core/src/main/java/io/questdb/griffin/engine/functions/window/LagDoubleFunctionFactory.java
📚 Learning: 2025-11-19T12:21:00.062Z 
Learnt from: jerrinot
Repo: questdb/questdb PR: 6413
File: core/src/test/java/io/questdb/test/cutlass/pgwire/PGJobContextTest.java:11982-12002
Timestamp: 2025-11-19T12:21:00.062Z
Learning: QuestDB Java tests use a deterministic random seed. The test utilities (e.g., io.questdb.test.tools.TestUtils and io.questdb.std.Rnd) produce reproducible sequences, so rnd_* functions (including rnd_uuid4) yield deterministic outputs across runs. Do not flag tests in core/src/test/** that assert against values produced by rnd_* as flaky due to randomness.

Applied to files:

  • core/src/main/java/io/questdb/griffin/engine/functions/rnd/RndDoubleArrayFunctionFactory.java
  • core/src/test/java/io/questdb/test/griffin/engine/functions/rnd/LongSequenceTest.java
  • core/src/test/java/io/questdb/test/cairo/ArrayTest.java
📚 Learning: 2025-08-04T09:16:27.366Z 
Learnt from: jerrinot
Repo: questdb/questdb PR: 5996
File: core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java:350-356
Timestamp: 2025-08-04T09:16:27.366Z
Learning: In QuestDB ILP tests, maximum dimension values like `(1 << 28) - 1` are intentionally hard-coded instead of using constants like `ArrayView.DIM_MAX_LEN` to serve as "canaries" that will cause test failures if the API limits change, ensuring developers are aware of breaking changes to the ILP API.

Applied to files:

  • core/src/test/java/io/questdb/test/cairo/ArrayTest.java
🧬 Code graph analysis (15)
core/src/main/java/io/questdb/griffin/engine/functions/window/LeadDoubleFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/rnd/RndDoubleArrayFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/test/java/io/questdb/test/cairo/ColumnTypeTest.java (10)
core/src/main/java/io/questdb/std/Rnd.java (1)
  • Rnd (39-481)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/constants/NullConstant.java (1)
  • NullConstant (46-296)
core/src/main/java/io/questdb/griffin/engine/functions/constants/GeoByteConstant.java (1)
  • GeoByteConstant (33-57)
core/src/main/java/io/questdb/griffin/engine/functions/constants/GeoShortConstant.java (1)
  • GeoShortConstant (33-58)
core/src/main/java/io/questdb/griffin/engine/functions/constants/GeoIntConstant.java (1)
  • GeoIntConstant (33-58)
core/src/main/java/io/questdb/griffin/engine/functions/constants/GeoLongConstant.java (1)
  • GeoLongConstant (33-58)
core/src/main/java/io/questdb/griffin/engine/functions/constants/NullBinConstant.java (1)
  • NullBinConstant (33-56)
core/src/main/java/io/questdb/griffin/engine/functions/constants/Long128Constant.java (1)
  • Long128Constant (32-63)
core/src/main/java/io/questdb/griffin/engine/functions/constants/IPv4Constant.java (1)
  • IPv4Constant (32-59)
core/src/main/java/io/questdb/griffin/engine/functions/window/LagTimestampFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/rnd/LongSequenceFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/test/java/io/questdb/test/cairo/ArrayTest.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/window/LagDateFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/window/LeadTimestampFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/window/LagLongFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/window/LeadDateFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/window/LagDoubleFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
core/src/main/java/io/questdb/griffin/engine/functions/window/LeadLongFunctionFactory.java (1)
core/src/main/java/io/questdb/cairo/ColumnType.java (1)
  • ColumnType (58-1153)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (35)
  • GitHub Check: New pull request (SelfHosted Running tests with cover on linux-other)
  • GitHub Check: New pull request (SelfHosted Running tests with cover on linux-pgwire)
  • GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-sub)
  • GitHub Check: New pull request (SelfHosted Running tests with cover on linux-cairo-root)
  • GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz2)
  • GitHub Check: New pull request (SelfHosted Running tests with cover on linux-fuzz1)
  • GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-sub)
  • GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
  • GitHub Check: New pull request (SelfHosted Running tests with cover on linux-griffin-root)
  • GitHub Check: New pull request (SelfHosted Griffin tests on linux-x64-zfs)
  • GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
  • GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
  • GitHub Check: New pull request (SelfHosted Other tests on linux-x86-graal)
  • GitHub Check: New pull request (Hosted Running tests on windows-other-2)
  • GitHub Check: New pull request (Hosted Running tests on windows-other-1)
  • GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
  • GitHub Check: New pull request (Hosted Running tests on windows-cairo-2)
  • GitHub Check: New pull request (Hosted Running tests on windows-cairo-1)
  • GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
  • GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
  • GitHub Check: New pull request (Hosted Running tests on windows-griffin-sub)
  • GitHub Check: New pull request (Hosted Running tests on windows-griffin-base)
  • GitHub Check: New pull request (Hosted Running tests on mac-other)
  • GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
  • GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
  • GitHub Check: New pull request (Hosted Running tests on mac-cairo)
  • GitHub Check: New pull request (SelfHosted Cairo tests on linux-x64-zfs)
  • GitHub Check: New pull request (SelfHosted Griffin tests on linux-x86-graal)
  • GitHub Check: New pull request (SelfHosted Griffin tests on linux-arm64)
  • GitHub Check: New pull request (Hosted Running tests on mac-griffin)
  • GitHub Check: New pull request (SelfHosted Cairo tests on linux-x86-graal)
  • GitHub Check: New pull request (SelfHosted Cairo tests on linux-arm64)
  • GitHub Check: New pull request (Check Changes Check changes)
  • GitHub Check: build
  • GitHub Check: build
🔇 Additional comments (18)
core/src/main/java/io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java (2)

160-162: LGTM! More precise type checking for array indices.

The change from a broad assignability check to isSameOrBuiltInWideningCast correctly restricts array indices to LONG or types with built-in widening casts to LONG (e.g., INT, SHORT, BYTE). This prevents implicit casting issues mentioned in the PR objectives, such as mixed numeric index types like arr[1, 3.0].

164-171: LGTM! Improved error message readability.

Using ColumnType.nameOf(argType) instead of the raw type code provides users with a human-readable type name in error messages (e.g., "DOUBLE" instead of "8").

core/src/main/java/io/questdb/griffin/engine/functions/window/LeadLongFunctionFactory.java (1)

68-70: LGTM! Fixes the lead/lag default value type mismatch.

This change correctly prevents DOUBLE from being used as a default value for LONG, addressing one of the internal error reproductions mentioned in the PR objectives. The new check ensures only LONG or types with built-in widening casts to LONG (e.g., INT, SHORT, BYTE) are accepted.

core/src/main/java/io/questdb/griffin/engine/functions/window/LagDateFunctionFactory.java (1)

67-69: Types that can widen to DATE are correctly restricted to NULL only.

The implementation in ColumnType.isBuiltInWideningCast (lines 460-475) confirms that numeric types (BYTE, SHORT, CHAR) cannot widen to DATE due to explicit exclusions on lines 465-467. Only NULL (line 470) can widen to DATE, and TIMESTAMP/LONG correctly widen FROM DATE to LONG (lines 472-473), not the reverse. The validation in LagDateFunctionFactory (line 67) and LeadDateFunctionFactory (line 68) correctly uses isSameOrBuiltInWideningCast, accepting only DATE itself or NULL as valid default values, which fixes the false positives mentioned in the PR description.

core/src/main/java/io/questdb/griffin/engine/functions/window/LeadTimestampFunctionFactory.java (1)

69-71: Verify TIMESTAMP variant compatibility—critical gap discovered.

The change correctly tightens validation and fixes the false positives (BYTE→TIMESTAMP, SHORT→TIMESTAMP, CHAR→TIMESTAMP). However, verification reveals an undocumented limitation: TIMESTAMP_NANO and TIMESTAMP_MICRO are treated as incompatible types by isBuiltInWideningCast.

When a column is TIMESTAMP_NANO and the default value is TIMESTAMP_MICRO (or vice versa), validation fails because both variants have the same base tag (8) but different encoded values, and there is no special handling for TIMESTAMP precision variant conversion in the widening cast logic. This means:

  • Column ts TIMESTAMP_NANO with default value cast(x as TIMESTAMP) (microseconds) → rejected
  • No explicit test coverage exists for this scenario in window function tests

The error message "default value must be can cast to timestamp" is also misleading, as it implies broader compatibility than actually supported—it requires the same TIMESTAMP precision.

Confirm whether TIMESTAMP variant incompatibility is intentional or requires documentation/test coverage.

core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java (1)

749-754: isConvertibleFrom usage and argument order look correct

Using ColumnType.isConvertibleFrom(func.getType(), type) here aligns with the documented (fromType, toType) contract and enforces that only constant/runtime‑constant expressions that are actually convertible to the required target type are accepted before proceeding, which matches the intent of coerceRuntimeConstantType.

core/src/main/java/io/questdb/griffin/engine/functions/rnd/RndDoubleArrayFunctionFactory.java (1)

110-127: Integer argument validation correctly aligned with built‑in widening semantics

Switching to ColumnType.isSameOrBuiltInWideningCast(arg.getType(), ColumnType.LONG) before calling getLong(null) matches the new contract tests and ensures we only accept args with a safe built‑in long getter, while the explicit 0..Integer.MAX_VALUE range check keeps overflow handling central and predictable. LGTM.

core/src/main/java/io/questdb/griffin/engine/functions/window/LagDoubleFunctionFactory.java (1)

67-70: Safe default‑value validation for DOUBLE lag

The new isSameOrBuiltInWideningCast(defaultValue.getType(), ColumnType.DOUBLE) check correctly guarantees that defaultValue.getDouble(record) is only used for types with a built‑in double getter, preventing internal UnsupportedOperationExceptions. Looks good.

core/src/main/java/io/questdb/griffin/engine/functions/window/LeadDoubleFunctionFactory.java (1)

67-70: Lead default‑value check consistent with DOUBLE semantics

Using ColumnType.isSameOrBuiltInWideningCast(defaultValue.getType(), ColumnType.DOUBLE) here mirrors the lag implementation and ensures only types with a safe built‑in double getter are accepted as defaults. No issues spotted.

core/src/main/java/io/questdb/griffin/engine/functions/window/LagLongFunctionFactory.java (1)

67-70: Long default‑value validation correctly narrowed to built‑in casts

The switch to ColumnType.isSameOrBuiltInWideningCast(defaultValue.getType(), ColumnType.LONG) is appropriate given we call defaultValue.getLong(record) in the implementation. This avoids internal errors for types that claimed generic assignability but lack a long getter.

core/src/main/java/io/questdb/griffin/engine/functions/window/LagTimestampFunctionFactory.java (1)

67-71: Timestamp default validation correctly tied to argument’s timestamp flavor

Validating with ColumnType.isSameOrBuiltInWideningCast(defaultValue.getType(), args.getQuick(0).getType()) aligns the default with the actual timestamp flavor of the first argument and ensures defaultValue.getTimestamp(record) integrates safely with TimestampDriver.from(..). This should eliminate the previous ambiguous‑cast/UOE corner cases.

core/src/main/java/io/questdb/griffin/engine/functions/rnd/LongSequenceFunctionFactory.java (2)

65-78: Graceful handling of non‑integral single argument types

Using ColumnType.isConvertibleFrom(countFunc.getType(), ColumnType.LONG) as a guard and then translating any UnsupportedOperationException from countFunc.getLong(null) into a SqlException with a clear “argument type X is not supported” message addresses the internal‑error issue cleanly while still allowing all long‑convertible constants. The slight redundancy between the convertibility check and the try/catch is acceptable here and keeps error messages more targeted than a pure try‑only approach.

If you want to tighten the contract further, you could consider switching this branch to isSameOrBuiltInWideningCast as well and relying less on the UnsupportedOperationException path, but that would narrow which implicit casts are accepted and might be a behavioral change—worth double‑checking against existing queries and tests.

80-93: Seeded variant correctly restricted to built‑in long widenings

For the 3‑argument seeded form, requiring ColumnType.isSameOrBuiltInWideningCast(_, ColumnType.LONG) on count, seedLo, and seedHi before calling getLong(null) ensures that only types with a guaranteed long getter participate, which matches the strengthened isBuiltInWideningCast contract and removes the need for defensive exception handling here. The semantics between unseeded and seeded variants are now intentionally different but consistent with the broader type‑system refactor.

core/src/main/java/io/questdb/griffin/engine/functions/window/LeadDateFunctionFactory.java (1)

67-70: Date default‑value check aligned with DATE getter contract

The move to ColumnType.isSameOrBuiltInWideningCast(defaultValue.getType(), ColumnType.DATE) matches the later use of defaultValue.getDate(record) and avoids advertising castability when no date getter exists. This looks correct and consistent with the other window factories.

core/src/test/java/io/questdb/test/griffin/engine/functions/rnd/LongSequenceTest.java (1)

40-43: LGTM!

The factory override is correctly implemented according to the test framework pattern.

core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java (3)

764-764: LGTM!

The change from isAssignableFrom to isConvertibleFrom correctly clarifies the semantics for timestamp UPDATE cast validation. The method now explicitly checks for any available conversion path, which aligns with the PR's objective to fix implicit casting issues.

2783-2783: LGTM!

The conversion checks in the INSERT AS SELECT flow now correctly use isConvertibleFrom, which checks for any available conversion path (including narrowing casts). This is appropriate since the code later generates the necessary RecordToRowCopier to handle the actual data conversion, and incompatible types are properly rejected with clear error messages.

Also applies to: 2839-2839

4038-4039: LGTM!

The UPDATE operation type validation correctly uses isConvertibleFrom for checking string/varchar compatibility. This allows the necessary flexibility for UPDATE statements while maintaining proper error reporting for truly incompatible types. The changes are consistent with the broader refactoring in this PR.

@puzpuzpuz puzpuzpuz self-requested a review November 25, 2025 12:14
@bluestreak01
Copy link
Member

bluestreak01 commented Nov 25, 2025

@jerrinot please send Ent tandem

@jerrinot
Copy link
Contributor Author

jerrinot commented Nov 27, 2025

@bluestreak01 done: https://github.com/questdb/questdb-enterprise/pull/797

@puzpuzpuz
Copy link
Contributor

puzpuzpuz commented Nov 27, 2025

@jerrinot you're still unlucky:

[ERROR] Errors: 
[ERROR]   WindowFunctionTest.testLeadLagTimestampMixedDefaultValue:4294->AbstractCairoTest.assertMemoryLeak:1219->AbstractCairoTest.assertMemoryLeak:1236->AbstractCairoTest.lambda$assertMemoryLeak$9:1241->lambda$testLeadLagTimestampMixedDefaultValue$23:4302->AbstractCairoTest.assertQuery:1894->AbstractCairoTest.assertMemoryLeak:1219->AbstractCairoTest.assertMemoryLeak:1236->AbstractCairoTest.lambda$assertMemoryLeak$9:1241->AbstractCairoTest.lambda$assertQuery$16:1894->AbstractCairoTest.assertQueryFullFatNoLeakCheck:1960->AbstractCairoTest.assertQueryNoLeakCheck:1974 » Sql [30] default value must be can cast to timestamp

puzpuzpuz
puzpuzpuz reviewed Nov 27, 2025
View reviewed changes
@jerrinot
Copy link
Contributor Author

jerrinot commented Nov 28, 2025

@puzpuzpuz the failing test is now fixed

@puzpuzpuz puzpuzpuz self-requested a review November 28, 2025 13:36
@glasstiger
Copy link
Contributor

glasstiger commented Nov 28, 2025

[PR Coverage check]

😍 pass : 39 / 40 (97.50%)

file detail

path covered line new line coverage
🔵 io/questdb/cairo/ColumnType.java 16 17 94.12%
🔵 io/questdb/griffin/engine/functions/window/LeadDateFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/window/LeadLongFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/SqlCompilerImpl.java 6 6 100.00%
🔵 io/questdb/griffin/SqlCodeGenerator.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/rnd/LongSequenceFunctionFactory.java 4 4 100.00%
🔵 io/questdb/griffin/engine/functions/rnd/RndDoubleArrayFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/window/LagLongFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/conditional/SwitchFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/window/LagDoubleFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/window/LagTimestampFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/array/DoubleArrayAccessFunctionFactory.java 2 2 100.00%
🔵 io/questdb/griffin/engine/functions/window/LeadDoubleFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/window/LeadTimestampFunctionFactory.java 1 1 100.00%
🔵 io/questdb/griffin/engine/functions/window/LagDateFunctionFactory.java 1 1 100.00%

@bluestreak01 bluestreak01 merged commit 63bdfc5 into master Nov 28, 2025
43 checks passed
@bluestreak01 bluestreak01 deleted the jh_coltype_castign branch November 28, 2025 14:53
@coderabbitai coderabbitai bot mentioned this pull request Dec 11, 2025
Merged
15 tasks
This was referenced Mar 4, 2026
Merged
Open
@coderabbitai coderabbitai bot mentioned this pull request Mar 27, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes

Assignees

No one assigned

Labels

Bug Incorrect or unexpected behavior

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

UnsupportedOperationException in RndDoubleArrayFunctionFactory

4 participants

@jerrinot @bluestreak01 @puzpuzpuz @glasstiger