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
Add support for processing Photon event logs in Scala #1338
base: dev
Are you sure you want to change the base?
Add support for processing Photon event logs in Scala #1338
Changes from 4 commits
98506a7
316c342
547ddc2
5dffd0c
941da22
cd4e32f
9bc9d10
df8f4ce
c129687
64b1a96
5d2753c
80440c7
a3d6a08
a73428d
84828c9
b19f3d1
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.
so is this implying the ones that have multiple entries in the mapping file aren't actually used? seems like a must fix
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 question here about selecting the first match, is there a plan to fix later?
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.
Discussed offline with Tom, for operators that have 1-to-many mappings, we cannot distinguish between them. Selecting the first one for now.
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.
does the meta all lineup and metrics? I wouldn't expect metrics to be anywhere close
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.
Could you explain the lining up and metrics?
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.
lets talk about off line. I mean that if photon execs have certain parameters, we need to make sure the CPU one we replace it with has its parameters filled in correctly. In addition to just parameters there are metrics that come out with each exec. In general the Photon ones I think keep the CPU ones and add their own but we need to verify that, especially if we use those metrics for other heuristics.
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.
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 perhaps goes to my last comment, it seems we aren't really doing the full mapping with parameters and everything? if we are going to convert it seems more like to convert it fully over to have some format if possible. If its not possible then we need something like shim layer to deal with.
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.
PhotonBroadcastNestedLoopJoinExecParser
)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.
not sure why this is here? Are we printing the photon plan or the converted plan or do we have options for both?
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.
Removed this
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.
Is it part of the requirements to add a binary flag for "Photon App" in the output files?
If not, then this should be part of the App properties Map[String, String]. For example, a user running on dataproc, the columns will not be useful.
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.
Removed the binary flag as we can detect this information from
spark_properties.csv
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 like to see this sooner rather then later if we can easily determine up front
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.
Changed the flow. We do not create a new instance of
SparkPlanInfo
here. We are only interested in thename