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

more complex visit_Call to parse chained command #5677

Merged
merged 3 commits into from Aug 24, 2022
Merged

Conversation

ChenyuLInx
Copy link
Contributor

resolves #5646

Description

Added more node traversing method to handle new test cases added in the unittest

Checklist

@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

1 similar comment
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@ChenyuLInx
Copy link
Contributor Author

Did some rough benchmark on performance implication.

For the the previous case in unittest, visiting the tree 10000 times, time with previous method takes 0.34s, new method takes 0.34s, as there's no additional visit needed.

For the test case in unittest, visiting the tree 10000 times, time with previous method takes 0.34s, new method takes 0.71s.

With the whole content in run.py, visiting 10000 times takes 7.65s for original method, takes 8.23s for new method.

Conclusion

The new method will slow things down but don't seems to cause performance concern.

func_name = func_name.split(".")[-1]
args, kwargs = self._get_call_literals(node)
self.dbt_function_calls.append((func_name, args, kwargs))
if isinstance(node.func, ast.Attribute) and node.func.attr in dbt_function_key_words:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gshank Do you have any experience with language parser? feels like we might be re-inventing the wheels now

@ChenyuLInx
Copy link
Contributor Author

Another thought for this is that there are tools that can convert regex to a language parser I think @jtcohen6 (the book we are reading mentioned it). Maybe at some point we can look into switch if performance/complex rules become a issue

@ChenyuLInx ChenyuLInx requested a review from gshank August 19, 2022 22:31
Copy link
Contributor

@gshank gshank left a comment

Choose a reason for hiding this comment

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

The AST based stuff gets weirdly complex. I'm not really sure if there's a way to simplify it; I kind of suspect that it's inherently complicated.

@ChenyuLInx ChenyuLInx merged commit 436737d into main Aug 24, 2022
@ChenyuLInx ChenyuLInx deleted the fix/chain_ref branch August 24, 2022 15:02
VersusFacit pushed a commit that referenced this pull request Sep 5, 2022
* more complex visit_Call

* add changelog

* traversing all of the tree
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request Sep 16, 2022
* more complex visit_Call

* add changelog

* traversing all of the tree
josephberni pushed a commit to Gousto/dbt-core that referenced this pull request Sep 16, 2022
* more complex visit_Call

* add changelog

* traversing all of the tree
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1034] [Bug] Unexpected behavior when chaining methods on dbt-ref'ed/sourced dataframes
2 participants