-
Notifications
You must be signed in to change notification settings - Fork 165
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: Use makeCopy
to change relation in FileSourceScanExec
#207
Conversation
bc12dc3
to
8f992c0
Compare
Ah, |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #207 +/- ##
=======================================
Coverage ? 33.33%
Complexity ? 769
=======================================
Files ? 107
Lines ? 35406
Branches ? 7673
=======================================
Hits ? 11804
Misses ? 21147
Partials ? 2455 ☔ View full report in Codecov by Sentry. |
@@ -439,8 +440,29 @@ case class CometScanExec( | |||
|
|||
object CometScanExec { |
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.
perhaps we can also consider CometScanExec extends TreeNode
to remove mapProductIterator
method copy?
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.
We want to run mapProductIterator
on FileSourceScanExec
. I'm not sure how object CometScanExec extends TreeNode
can help us?
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.
You are right, its needed to run mapProductIterator
on FileSourceScanExec
, I wrongly assumed this method needed for CometScanExec
.
This patch cannot completely resolve the ticket #190 but only fixes the first error. After the first error is fixed, there comes out another API incompatibility issue around I think it is still good to have this patch to fix the first 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.
lgtm thanks @viirya
Merged. Thanks. |
Which issue does this PR close?
Relates #190.
Rationale for this change
What changes are included in this PR?
How are these changes tested?