Skip to content
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 a new edge kind - ParentIterative #617

Merged
merged 5 commits into from
Sep 22, 2023

Conversation

ketkarameya
Copy link
Contributor

@ketkarameya ketkarameya commented Sep 20, 2023

Earlier we had Parent and this is how it was processed:

for rule in graph.outgoing(current_rule, PARENT):
   for ancestor in context(current_location):
       Edit edit = get_edit(rule, ancestor)
       if edit : 
          return edit 

For ParentIterative we do something subtly different. We loop over the ancestors in the outer most loop:
And its processed something like :

   for ancestor in context(current_location):
       for rule in graph.outgoing(current_rule, PARENT_ITERATIVE):
         Edit edit = get_edit(rule, ancestor)
         if edit : 
          return edit 

Why suffix iterative? I thought its keeping to iterative-deepening idea from DFS.
I am open to other names.
But this pattern makes it easy to transform builder patterns (As required by Scala upgrade folks)

@ketkarameya ketkarameya changed the title Add a new edge kind - ParentIterative Add a new edge kind - ParentIterative Sep 20, 2023
@ketkarameya ketkarameya force-pushed the ketkara-edge-parent-iterative branch from e951072 to 08da159 Compare September 20, 2023 23:45
Copy link
Contributor

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

Minor nits below, plus the offline request to add the negative test case, but otherwise this LGTM!

Comment on lines 25 to 26


Copy link
Contributor

Choose a reason for hiding this comment

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

Are this extra empty lines intentional?

replace_node = "*"
replace = 'new SparkSession.builder()'


Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit: but let's make the spacing between rules consistently, either one or two empty lines, but not a mix of the two unless it implies a particular grouping.

@@ -280,8 +267,8 @@ impl SourceCodeUnit {
stack: &mut VecDeque<(CGPattern, InstantiatedRule)>,
) {
for (scope_level, rules) in next_rules_by_scope {
// Scope level is not "PArent" or "Global"
if ![PARENT, GLOBAL].contains(&scope_level.as_str()) {
// Scope level is not "Parent" or "Global"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Scope level is not "Parent" or "Global"
// Scope level is not "Parent", "Parent Iterative", or "Global"

self.rewrites_mut().push(edit.clone());
debug!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, we are intentionally getting rid of this debug log line entirely, right?

@ketkarameya ketkarameya merged commit aa255dc into master Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants