-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Spark 3.5: Support recursive delegate unwrapping to find ExtendedParser in parser chains #14483
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
Conversation
…er in parser chains
…er in parser chains
|
@majian1998 Is this a clean back-port? |
@huaxingao Yes, just a straight copy. |
huaxingao
left a comment
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.
LGTM
| ParserInterface next = getNextDelegateParser(current); | ||
| if (next == null) { | ||
| break; | ||
| } | ||
|
|
||
| current = next; |
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.
| ParserInterface next = getNextDelegateParser(current); | |
| if (next == null) { | |
| break; | |
| } | |
| current = next; | |
| current = getNextDelegateParser(current); |
| clazz = clazz.getSuperclass(); | ||
| } | ||
| } catch (Exception e) { | ||
| // ignore |
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.
shouldn't we at least log something?
| } catch (NoSuchFieldException e) { | ||
| clazz = clazz.getSuperclass(); | ||
| } |
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.
add log to help RCA if something goes wrong
| } catch (NoSuchFieldException e) { | ||
| clazz = clazz.getSuperclass(); | ||
| } |
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.
same as above
| when(icebergParser.parseSortOrder("id ASC NULLS FIRST")).thenReturn(expected); | ||
|
|
||
| ParserInterface wrapper = new WrapperParser(icebergParser); | ||
|
|
||
| setSessionStateParser(spark.sessionState(), wrapper); | ||
|
|
||
| List<ExtendedParser.RawOrderField> result = | ||
| ExtendedParser.parseSortOrder(spark, "id ASC NULLS FIRST"); | ||
| assertThat(result).isSameAs(expected); | ||
|
|
||
| verify(icebergParser).parseSortOrder("id ASC NULLS FIRST"); |
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.
| when(icebergParser.parseSortOrder("id ASC NULLS FIRST")).thenReturn(expected); | |
| ParserInterface wrapper = new WrapperParser(icebergParser); | |
| setSessionStateParser(spark.sessionState(), wrapper); | |
| List<ExtendedParser.RawOrderField> result = | |
| ExtendedParser.parseSortOrder(spark, "id ASC NULLS FIRST"); | |
| assertThat(result).isSameAs(expected); | |
| verify(icebergParser).parseSortOrder("id ASC NULLS FIRST"); | |
| when(icebergParser.parseSortOrder("id ASC NULLS FIRST")).thenReturn(expected); | |
| verify(icebergParser).parseSortOrder("id ASC NULLS FIRST"); | |
| ParserInterface wrapper = new WrapperParser(icebergParser); | |
| setSessionStateParser(spark.sessionState(), wrapper); | |
| List<ExtendedParser.RawOrderField> result = | |
| ExtendedParser.parseSortOrder(spark, "id ASC NULLS FIRST"); | |
| assertThat(result).isSameAs(expected); |
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 doesn’t work as expected—verify needs to go after you actually call the method. Otherwise, there’s nothing to verify yet
| when(icebergParser.parseSortOrder("id ASC NULLS FIRST")).thenReturn(expected); | ||
| ParserInterface parser = new GrandChildParser(icebergParser); | ||
| setSessionStateParser(spark.sessionState(), parser); | ||
|
|
||
| List<ExtendedParser.RawOrderField> result = | ||
| ExtendedParser.parseSortOrder(spark, "id ASC NULLS FIRST"); | ||
| assertThat(result).isSameAs(expected); | ||
| verify(icebergParser).parseSortOrder("id ASC NULLS FIRST"); |
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.
| when(icebergParser.parseSortOrder("id ASC NULLS FIRST")).thenReturn(expected); | |
| ParserInterface parser = new GrandChildParser(icebergParser); | |
| setSessionStateParser(spark.sessionState(), parser); | |
| List<ExtendedParser.RawOrderField> result = | |
| ExtendedParser.parseSortOrder(spark, "id ASC NULLS FIRST"); | |
| assertThat(result).isSameAs(expected); | |
| verify(icebergParser).parseSortOrder("id ASC NULLS FIRST"); | |
| when(icebergParser.parseSortOrder("id ASC NULLS FIRST")).thenReturn(expected); | |
| verify(icebergParser).parseSortOrder("id ASC NULLS FIRST"); | |
| ParserInterface parser = new GrandChildParser(icebergParser); | |
| setSessionStateParser(spark.sessionState(), parser); | |
| List<ExtendedParser.RawOrderField> result = | |
| ExtendedParser.parseSortOrder(spark, "id ASC NULLS FIRST"); | |
| assertThat(result).isSameAs(expected); |
|
This is a backport PR; let’s merge it unchanged to keep it identical to the previous one. |
|
Merged. Thanks @majian1998 for the PR! Thanks @singhpk234 @kamcheungting-db for the review! |
@huaxingao Got it, I’ll read through the comments and create a follow-up PR for 3.5 and 4.0. |
Related PR: #13625
Since support for Spark 3.4 will be removed in version 1.11, this fix is only applied to Spark 3.5.