Skip to content

Conversation

@beliefer
Copy link
Contributor

What changes were proposed in this pull request?

#35975 supported ANSI SQL: result offset clause.
We make some check for offset in CheckAnalysis.
The behavior prevents add offset API into Dataset.
So this PR move the check of Offset from analysis to finsh analysis.

Why are the changes needed?

Let we can add offset API into Dataset.

Does this PR introduce any user-facing change?

'No'.
New feature.

How was this patch tested?

New tests

@github-actions github-actions bot added the SQL label Apr 22, 2022
@beliefer
Copy link
Contributor Author

ping @dtenedor cc @cloud-fan

plan.foreachUp {
case o if !o.isInstanceOf[GlobalLimit] && !o.isInstanceOf[LocalLimit]
&& o.children.exists(_.isInstanceOf[Offset]) =>
throw new IllegalStateException(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a user-facing error, we should use AnalysisException

}

/**
* Validate whether the [[Offset]] is valid.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's explain the reason. Dataset API eagerly analyzes the query, so a query plan may contain invalid Offset operators but it's not the final query plan that gets evaluated.

@beliefer
Copy link
Contributor Author

Because #36417 merged.

@beliefer beliefer closed this May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants