Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[SPARK-28690][SQL] Add
date_part
function for timestamps/dates #25410[SPARK-28690][SQL] Add
date_part
function for timestamps/dates #25410Changes from 19 commits
cc81650
0e571be
b856859
d23b8ba
af51e52
e68611a
e1d5a75
ae353b9
f3b2772
b795ebc
efc3ee0
188d7de
9935c01
c918eb0
bcf73d2
7c549c7
1b2c8d4
2fa25b1
494679c
5c8c34d
05b3746
74a7fec
bc707df
d86c092
ade541d
43968c2
b292931
600eee6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception changed from
ParseException
toAnalysisException
? Can we keep the current behaviour?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseException
requires eitherctx: ParserRuleContext
orval start: Origin, val stop: Origin
that are not available to me at the point. And it is stillParseException
in the output: https://github.com/apache/spark/pull/25410/files#diff-6f4edc80e2cc973e748705e85a6053b4R514There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... but I can pass
ctx: ParserRuleContext
toDatePart
's constructor. I am just not sure that is is good practice. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing ctx into DataPart, can't we resolve
extractField
inAstBuilder
then pass the resolved into DataPart?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will work for
extract
but how about thedate_part()
function itself? The same code should present for the function, shouldn't it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ur, I see! sorry, but I need to be away from a keybord, so I'll recheck in hours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've heard that its a little hard to read parser error messages in spark. So, I personally think we should throw ParseException with ctx for the
extract
case as much as possible. Then, how about writing it like this? d793f49There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it would be nice to point out the exact position of the error but I don't like this ad-hoc solution. There are a few similar places in
datetimeExpressions
and others where we could throwParseException
instead of justAnalysisException
. I believe we need a common approach. Why do you think that propagation of the parsing context or positions look not good? We could propagate such parameters to all expression constructors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ur, I don't think the approach is bad and I'm just looking for a better solution to keep the current error handling. If we already have the similar logic, can we follow that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maropu @dongjoon-hyun If you think the commit d793f49 is necessary for merging the PR, I will apply the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. What I meant was backquoting for the special value like
54
.For the other values like
year
andmonth
, we don't need back quoting.So,
AS ".."
, we can use backquoting.AS xxx
, we don't need backquoting, too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will remove the backquoting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongjoon-hyun When I removed them, I got the error:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would guess,
year
belongs to a function as well. Probably, this confuses the parser?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongjoon-hyun I removed backquotes only around names that are not functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? @MaxGekk . The master branch seems to be same. Could you push your change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongjoon-hyun Pushed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the backquotes back and opened JIRA for the issue: https://issues.apache.org/jira/browse/SPARK-28767
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because
spark.sql.parser.ansi.enabled
is set tofalse
by default:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked that? #25114 (comment)
I think that's an expected behaviour in the ansi mode.