Skip to content

feat: improve type inference for WindowFrame#13059

Merged
goldmedal merged 3 commits intoapache:mainfrom
notfilippo:fr/better-type-inference-for-window-frames
Oct 25, 2024
Merged

feat: improve type inference for WindowFrame#13059
goldmedal merged 3 commits intoapache:mainfrom
notfilippo:fr/better-type-inference-for-window-frames

Conversation

@notfilippo
Copy link
Copy Markdown
Member

@notfilippo notfilippo commented Oct 22, 2024

Which issue does this PR close?

Closes #11432
Closes #12982

Rationale for this change

This tried to fix the liked issue by improving the type inference when creating window frame from sqlparser.

What changes are included in this PR?

  • WindowFrameBound's try_from method is replaced by a try_parse that also takes the WindowFrame's units in order to make some conclusions about the type.

Are these changes tested?

Yes

Are there any user-facing changes?

No API changes but potentially a new behaviour introduced that could break some existing code.

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate labels Oct 22, 2024
Comment thread datafusion/expr/src/window_frame.rs Outdated
Comment thread datafusion/sqllogictest/test_files/window.slt
Comment thread datafusion/sql/tests/cases/plan_to_sql.rs
@jcsherin
Copy link
Copy Markdown
Contributor

The substrait plan in the original reproducer now contains the expected window frame values for ROWS BETWEEN 1 PRECEDING AND 1 FOLLOWING:

bounds_type: Rows,
lower_bound: Some(
    Bound {
        kind: Some(
            Preceding(
                Preceding {
                    offset: 1,
                }
            ),
        ),
    },
),
upper_bound: Some(
    Bound {
        kind: Some(
            Following(
                Following {
                    offset: 1,
                }
            ),
        ),
    },
),

@jcsherin
Copy link
Copy Markdown
Contributor

@notfilippo Thanks, squashed three bugs with this refactor 🚀.

cc @alamb

Copy link
Copy Markdown
Contributor

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

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

Thanks @notfilippo -- overall looks good to me but I got some problems when trying to add more sqllogictests.

Comment thread datafusion/expr/src/window_frame.rs Outdated
Comment thread datafusion/sqllogictest/test_files/window.slt
Comment thread datafusion/expr/src/window_frame.rs Outdated
Comment thread datafusion/substrait/src/logical_plan/producer.rs
@notfilippo notfilippo requested a review from goldmedal October 23, 2024 19:36
Copy link
Copy Markdown
Contributor

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

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

Thanks @notfilippo and @jcsherin for reviewing. I just have one minor question #13059 (comment) . Others LGTM 👍.

@notfilippo
Copy link
Copy Markdown
Member Author

@goldmedal I've removed SingleQuotedString according to #13059 (comment).

@goldmedal
Copy link
Copy Markdown
Contributor

Thanks @notfilippo
If no more comments from the other one, I plan to merge this tomorrow.

@goldmedal goldmedal merged commit 6a3c0b0 into apache:main Oct 25, 2024
@goldmedal
Copy link
Copy Markdown
Contributor

Thanks again @notfilippo @jcsherin 👍

@notfilippo notfilippo deleted the fr/better-type-inference-for-window-frames branch October 25, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner sqllogictest SQL Logic Tests (.slt) substrait Changes to the substrait crate

Projects

None yet

3 participants