-
Notifications
You must be signed in to change notification settings - Fork 16.3k
feat(provider): pass parameters to Neo4j driver session #52990
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
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
|
Tests are failing |
|
@BBQing The review comments are addressed. Please review, Thanks !! |
eladkal
left a comment
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 have some concerns about this feature/bug-fix?
When using the Neo4jOperator to execute cypher queries, the parameters argument is not being passed correctly. The operator is designed to accept a parameters argument, but this argument is not utilized.
This suggest a bug fix as the claim is that parameters is not working but this parameter doesn't exist. This PR is what adding it.
I am not saying no to this PR but I am requesting some clarity here.
Please describe what is exactly missing, What real-life use case can not be served properly with the current operator.
|
@rmidhun23 now it looks fine to me. I have only one question - in the PR description you have claimed successful result, yet the submitted code contained bugs - very simple and obvious bugs. How did it happen to you? What went wrong in your workflow? Just out of curiosity. |
|
hi @eladkal. The issue is that the The operator supports only for static queries and you don't have an option to use dynamic values in query. An example use-case is where one relies on upstream data. Let me know if this clarifies my intentions with this fix. |
@BBQing Thanks for helping me out. I was able to verify its working fine from the logs which i have shared here (but I created a copy of the operator as a custom module with the changes and included my dag deployment). |
Thank you for your answer. |
13a2242 to
e687699
Compare
@eladkal Would like your comments on this, Thanks |
e687699 to
5cd5f75
Compare
New feature for me. |
eladkal
left a comment
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
Would be good to mention this also in the docs with reference to the Neo4j docs. |
Maybe add/modify example ? @rmidhun23 ? |
@potiuk @eladkal Sure, will add an example to the docs and get back to you. |
@potiuk @eladkal Please review the docs content and let me know your thoughts. |
|
static checks fail |
5657157 to
ad78ab0
Compare
Closes: #52723
Following up with a PR to fix issue.
Why
When using the
Neo4jOperatorto execute cypher queries, theparametersargument is not being passed correctly. The operator is designed to accept aparametersargument, but this argument is not utilized.The
Neo4jOperatorshould correctly pass theparametersargument to the cypher query execution. This would allow for dynamic queries that can accept parameters at runtime.How
Neo4jHookhook to acceptparameterswhen operator is executed.Pass
parametersargument toNeo4jdriver session for queries with schema or schemaless operations.Add
testsfor coverage.Deployment Details
Result
Logs
Dag UI
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.