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

feat: add multi-query transaction support to snowflake piece #92

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

valentin-mourtialon
Copy link

@valentin-mourtialon valentin-mourtialon commented Jan 24, 2025

What does this PR do?

Enhances the Snowflake piece's query execution capabilities by:

  • Adding support for executing multiple SQL queries sequentially
  • Introducing optional transaction handling (configurable via checkbox)
  • Returning detailed results for each executed query
  • Maintaining data consistency with automatic rollback on failure when transactions are enabled

@valentin-mourtialon valentin-mourtialon force-pushed the valentin-mourtialon/feat-snowflake-multiple-queries-handling branch 2 times, most recently from 522e748 to 0f20e53 Compare January 24, 2025 14:37
@valentin-mourtialon valentin-mourtialon marked this pull request as ready for review January 24, 2025 14:40
@valentin-mourtialon valentin-mourtialon self-assigned this Jan 24, 2025
'Array of SQL queries to execute in order, in the same transaction. Use :1, :2… or ? placeholders to use binding parameters.',
required: true,
}),
binds: Property.Array({

Choose a reason for hiding this comment

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

The binds are shared across all queries

  • does it work if 1 query does not use all the binds? no error?
  • does snowflake support named binding params? to avoid any surprise
  • using ? seems dangerous, at least :1 :2 etc. should be deterministic - we should at least update the description to nudge towards the best practice

Have you tested a few different scenarios?

If we don't have an easy solution to prevent such mistakes, an alternative would be to have binds per query - it's a bit more complex but totally possible (Property.Array can be more complex)

Choose a reason for hiding this comment

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

Re. named params, looks like the answer is no:
https://stackoverflow.com/questions/76814603/how-can-i-use-named-binding-in-nodejs-while-pulling-data-from-snowflake
(not surprising, I had checked last year already)

Copy link
Author

@valentin-mourtialon valentin-mourtialon Jan 24, 2025

Choose a reason for hiding this comment

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

does it work if 1 query does not use all the binds? no error?

yes

Have you tested a few different scenarios?

Tested combinations:

  1. Unused binds
  2. All binds used by query number 1
  3. Multiple binds in a query
  4. Single bind in a query
  5. Different query, different bind (Query 1 uses bind 1, Query 2 uses bind 2)
  6. Int binds
  7. String binds

Did I miss anything?

Copy link
Author

@valentin-mourtialon valentin-mourtialon Jan 24, 2025

Choose a reason for hiding this comment

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

If we don't have an easy solution to prevent such mistakes, an alternative would be to have binds per query

I find the way it currently works pretty clear. 🤔 I mean, it works as we could naively expect it to work.

I've updated the description btw.

Choose a reason for hiding this comment

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

We could probably ask the AP team for advice about this WDYT?

@valentin-mourtialon valentin-mourtialon force-pushed the valentin-mourtialon/feat-snowflake-multiple-queries-handling branch from 0f20e53 to bef5b0d Compare January 24, 2025 18:11
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