Skip to content
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

fix: Support double quotes in date_part #10833

Merged
merged 8 commits into from
Jun 10, 2024

Conversation

Weijun-H
Copy link
Member

@Weijun-H Weijun-H commented Jun 8, 2024

Which issue does this PR close?

Parts #10826

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jun 8, 2024
@@ -127,7 +127,7 @@ impl ScalarUDFImpl for DatePartFunc {
ColumnarValue::Scalar(scalar) => scalar.to_array()?,
};

let arr = match part.to_lowercase().as_str() {
let arr = match part.to_lowercase().trim_matches('\'').trim_matches('"') {
Copy link
Member Author

Choose a reason for hiding this comment

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

key change

Copy link
Contributor

Choose a reason for hiding this comment

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

@Weijun-H Weijun-H changed the title fix: Support double quotes in date_part.rs fix: Support double quotes in "date_part" Jun 8, 2024
@Weijun-H Weijun-H changed the title fix: Support double quotes in "date_part" fix: Support double quotes in date_part Jun 8, 2024
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.

Thank you @Weijun-H 🙏

Looks like there are a few test failures to resolve but otherwise 👍

@@ -127,7 +127,7 @@ impl ScalarUDFImpl for DatePartFunc {
ColumnarValue::Scalar(scalar) => scalar.to_array()?,
};

let arr = match part.to_lowercase().as_str() {
let arr = match part.to_lowercase().trim_matches('\'').trim_matches('"') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Just curious, does this let you get away with something funky like:

select extract('''epoch''' from now());

@Weijun-H
Copy link
Member Author

Weijun-H commented Jun 8, 2024

Just curious, does this let you get away with something funky like:

select extract('''epoch''' from now());

It seems the invalid input @Jefffrey

DataFusion CLI v39.0.0
> select extract('''epoch''' from now());  🤔 Invalid statement: sql parser error: Expected date/time field, found: ''epoch''

@Jefffrey
Copy link
Contributor

Jefffrey commented Jun 8, 2024

Just curious, does this let you get away with something funky like:

select extract('''epoch''' from now());

It seems the invalid input @Jefffrey

DataFusion CLI v39.0.0
> select extract('''epoch''' from now());  🤔 Invalid statement: sql parser error: Expected date/time field, found: ''epoch''

Ah whoops. What about:

select extract("''''epoch''''" from now());

FWIW this is how Postgres treats it:

postgres=# select extract("''''epoch''''" from now());
ERROR:  unit "''''epoch''''" not recognized for type timestamp with time zone

@Weijun-H Weijun-H requested review from Jefffrey and alamb June 9, 2024 06:26
Comment on lines 130 to 134
let arr = match part
.to_lowercase()
.replacen('\'', "", 2)
.replacen('\"', "", 2)
.as_str()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this will still let select extract("'epoch'" from now()); through

But this is quite a minor thing, not sure how much it matters in the wider picture 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

I found a workaround, but it should be a better way to handle it in sqlparser. Because it should have many use cases.

Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

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

Looks good, I agree would be nice to upstream to sqlparser in future if possible 👍

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.

Thank you @Weijun-H and @Jefffrey 🙏

@alamb alamb merged commit e094f94 into apache:main Jun 10, 2024
23 checks passed
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
* fix: Support double quotes in date_part.rs

* chore

* chore

* refacor: Use Regex to extract

* chore

* chore: Use replaceen

* chore

* fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants