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

fix: insert and delete executor #611

Merged
merged 8 commits into from
Mar 28, 2023
Merged

fix: insert and delete executor #611

merged 8 commits into from
Mar 28, 2023

Conversation

aryan-rajoria
Copy link
Collaborator

@aryan-rajoria aryan-rajoria commented Mar 16, 2023

Fixing issues in insert and delete executor based on feedback from @gaurav274

  • Using list comprehension in insert_executor.
  • Removing _roll_back from the executors.
  • Using descriptive variable names.
  • Applying predicate function in delete_executor using the SQLAlchemy.
  • Move single_predicate_to_filter() to ExpressionUtils.
  • Make it more robust (take into consideration AND and OR)
  • Adding test cases for single_predicate().

* fix to insert and delete executor

* variable names

* removing unnecessary arrays

* use SQLalchemy for delete execution predicates

* formatting

* moving delete_predicate to expression_utils

* formatting
@aryan-rajoria aryan-rajoria changed the title Insert feedback (#610) fix: insert and delete executor (#610) Mar 16, 2023
@aryan-rajoria aryan-rajoria changed the title fix: insert and delete executor (#610) fix: insert and delete executor (#611) Mar 16, 2023
@aryan-rajoria aryan-rajoria requested a review from gaurav274 March 16, 2023 19:10
@jarulraj
Copy link
Member

@aryan-rajoria single_predicate_node_to_filter is missing.

@jarulraj jarulraj changed the title fix: insert and delete executor (#611) fix: insert and delete executor Mar 26, 2023
@jarulraj jarulraj mentioned this pull request Mar 26, 2023
7 tasks
@jarulraj
Copy link
Member

@aryan-rajoria I brought the PR up-to-date with master. Please pull in the latest changes.

@jarulraj
Copy link
Member

This delete statement does not work -- """DELETE FROM testDeleteOne WHERE id < 20 AND dummyfloat < 2;"""

@jarulraj
Copy link
Member

This delete statement does not work -- """DELETE FROM testDeleteOne WHERE id < 20 AND dummyfloat < 2;"""

This is now resolved. All the tasks have been completed.

@jarulraj jarulraj requested a review from jiashenC March 27, 2023 04:10
elif predicate_node.etype == ExpressionType.COMPARE_LEQ:
filter_clause = x <= y
elif predicate_node.etype == ExpressionType.COMPARE_NEQ:
filter_clause = x != y
Copy link
Collaborator

Choose a reason for hiding this comment

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

CONTAINS and IS_CONTAINED are not supported here. We can check and raise exception if they are non-trivial.

Copy link
Member

Choose a reason for hiding this comment

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

Resolved.


filter_clause = predicate_node_to_filter_clause(
table=table_to_delete_from, predicate_node=where_clause
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I feel it is better to move predicate_node_to_filter_clause logic into delete_executor and the delete_executor accepts the ColumnElement[bool] as the input argument (i.e., where_clause). ColumnElement[bool] is the return type of and_ and or_

Copy link
Member

Choose a reason for hiding this comment

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

Good idea. Moved it to delete_executor

@gaurav274 gaurav274 merged commit 274dd26 into master Mar 28, 2023
@gaurav274 gaurav274 deleted the insert-feedback branch March 28, 2023 01:45
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.

4 participants