Skip to content

Conversation

@Jefffrey
Copy link
Contributor

Which issue does this PR close?

Closes #3183.

Rationale for this change

What changes are included in this PR?

Keep track of interval types found during parsing as bitflag.

Also refactor interval types to use an Enum to better serve above point.

Are these changes tested?

Added unit test.

Are there any user-facing changes?

@Jefffrey Jefffrey marked this pull request as ready for review November 12, 2022 11:42
@Jefffrey Jefffrey force-pushed the 3183_stricter_interval_syntax branch from 04d7742 to e222914 Compare November 12, 2022 20:27
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

This looks great to me -- thank you @Jefffrey

I had a small suggestion to make the error message more specific, and then this PR needs to be rebased, but otherwise I think it is good to go

cc @waynexia and @waitingkuo

Comment on lines 116 to 121
// Disallow duplicate interval types
if used_interval_types & (it as u16) != 0 {
return Err(DataFusionError::SQL(ParserError::ParserError(format!(
"Invalid input syntax for type interval: {:?}",
value
))));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Disallow duplicate interval types
if used_interval_types & (it as u16) != 0 {
return Err(DataFusionError::SQL(ParserError::ParserError(format!(
"Invalid input syntax for type interval: {:?}",
value
))));
// Disallow duplicate interval types
if used_interval_types & (it as u16) != 0 {
return Err(DataFusionError::SQL(ParserError::ParserError(format!(
"Invalid input syntax for type interval: {:?}. Repeated type {}",
value, interval_type
))));

@Jefffrey Jefffrey force-pushed the 3183_stricter_interval_syntax branch from e222914 to 522627b Compare November 15, 2022 22:10
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @Jefffrey and @alamb 👍

@alamb alamb merged commit d52234f into apache:master Nov 16, 2022
@alamb
Copy link
Contributor

alamb commented Nov 16, 2022

Thanks again @Jefffrey !

@ursabot
Copy link

ursabot commented Nov 16, 2022

Benchmark runs are scheduled for baseline = b7a33c9 and contender = d52234f. d52234f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@Jefffrey Jefffrey deleted the 3183_stricter_interval_syntax branch November 16, 2022 20:59
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.

INTERVAL literal with duplicated interval types should raise error

5 participants