Skip to content

Conversation

@HeartSaVioR
Copy link
Contributor

What changes were proposed in this pull request?

This PR reflects review comments on post-hoc review among PRs for SPARK-29779 (#27085), SPARK-30479 (#27164). The list of review comments this PR addresses are below:

I also applied review comments to the CORE module (BasicEventFilterBuilder.scala) as well, as the review comments for SQL/core module (SQLEventFilterBuilder.scala) can be applied there as well.

Why are the changes needed?

There're post-hoc reviews on PRs for such issues, like links in above section.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing UTs.

@HeartSaVioR
Copy link
Contributor Author

cc. @HyukjinKwon

@SparkQA
Copy link

SparkQA commented Jan 31, 2020

Test build #117639 has finished for PR 27414 at commit 9c78854.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @HeartSaVioR and @srowen .
Merged to master.

@HeartSaVioR
Copy link
Contributor Author

Thanks all for reviewing and merging!

FYI to @HyukjinKwon The PR only addressed part of your review comments, and I left my comment per comment to explain why I don't think it should be addressed. Please continue commenting on original comment if you don't agree with my comment.

@HeartSaVioR HeartSaVioR deleted the SPARK-28869-SPARK-29779-SPARK-30479-FOLLOWUP-posthoc-reviews branch January 31, 2020 23:09
Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM. thanks for this followup @HeartSaVioR, and all for reviewing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants