Skip to content

Bug fix: Fix issue where statements where not downcasted to IDataValue for case when #816

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

Merged
merged 1 commit into from
May 12, 2025

Conversation

Ulimo
Copy link
Contributor

@Ulimo Ulimo commented May 12, 2025

No description provided.

@Ulimo Ulimo requested a review from Copilot May 12, 2025 12:28
Copy link
Contributor

@Copilot Copilot AI left a 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 fixes an issue with CASE expressions not correctly downcasting to IDataValue. The changes include adding an acceptance test to cover the edge case and updating the expression visitor to enforce proper conversion of both the else and then branches to IDataValue.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/FlowtideDotNet.AcceptanceTests/SelectTests.cs Adds an acceptance test to validate downcasting in CASE expressions
src/FlowtideDotNet.Core/Compute/Columnar/ColumnarExpressionVisitor.cs Updates conversion logic to ensure both else and then expressions are cast to IDataValue
Comments suppressed due to low confidence (2)

src/FlowtideDotNet.Core/Compute/Columnar/ColumnarExpressionVisitor.cs:213

  • Verify that forcing a conversion on elseStatement using Expression.Convert correctly handles all value types (including the explicit use of new NullValue()) and preserves the intended evaluation semantics.
if (elseStatement.Type != typeof(IDataValue))

src/FlowtideDotNet.Core/Compute/Columnar/ColumnarExpressionVisitor.cs:235

  • Ensure that converting thenStatement to IDataValue via Expression.Convert does not inadvertently change its behavior; consider adding a clarifying code comment to maintain clarity on why the conversion is required.
if (thenStatement.Type != typeof(IDataValue))

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Benchmark

Benchmark suite Current: ede4514 Previous: 22d4209 Ratio
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.InnerJoin 416802780 ns (± 12561423.928290226) 458826533.3333333 ns (± 10640413.19005047) 0.91
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.LeftJoin 479855355.5555556 ns (± 23170147.270254407) 564994640 ns (± 28550338.329818636) 0.85
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ProjectionAndNormalization 165795080 ns (± 17950867.73710706) 169842820 ns (± 7144067.768715523) 0.98
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.SumAggregation 171794990 ns (± 13623705.568946114) 171581250 ns (± 12415362.812660772) 1.00
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ListAggWithMapAggregation 1805940440 ns (± 95011696.74410793) 1982296640 ns (± 123349811.75190067) 0.91
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.WindowSum 367513480 ns (± 15220572.565230403)
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ListAggWithStructAggregation 1415626060 ns (± 78487765.79391848)

This comment was automatically generated by workflow using github-action-benchmark.

@Ulimo Ulimo merged commit 58f08e1 into main May 12, 2025
8 checks passed
@Ulimo Ulimo deleted the case_fix_downcast branch May 12, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants