-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-29348][SQL][FOLLOWUP] Fix slight bug on streaming example for Dataset.observe #27046
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
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 was a minor bug on example: unlike observedMetrics available in QueryExecutionListener, event.progress.observedMetrics is a "Java" Map instead of "Scala" Map, hence to use foreach it needs asScala to convert to Scala Map.
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.
Sounds fine. The alternative is to wrap it in Option(....get(...)).foreach but that's messier
|
Thanks for reviewing, reflected review comments, as well as updated PR description to reflect the change. |
sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
Outdated
Show resolved
Hide resolved
HyukjinKwon
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.
Looks good by me
|
Test build #115935 has finished for PR 27046 at commit
|
|
Test build #115931 has finished for PR 27046 at commit
|
|
Test build #115936 has finished for PR 27046 at commit
|
|
retest this please |
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.
sorry last nit. this one 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.
FYI @hvanhovell as I believe you're already using this API; this changes the method signature.
|
See my comments on this in #26127. I am -1 on this in its current form. I am fine with fixing the example. |
|
For the record, the link of key comment is here: #26127 (comment) |
4c16552 to
2878cdb
Compare
|
Just updated. Please take a look again. Thanks in advance! |
|
Test build #115940 has finished for PR 27046 at commit
|
|
Discussed with @hvanhovell offline as well. Yup, I am good with it. |
|
Test build #115954 has finished for PR 27046 at commit
|
|
Merged to master. |
|
Thanks for reviewing and merging! |
What changes were proposed in this pull request?
This patch fixes a small bug in the example of streaming query, as the type of observable metrics is Java Map instead of Scala Map, so to use foreach it should be converted first.
Why are the changes needed?
Described above.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Ran below query via
spark-shell:Streaming