Skip to content

feat(NoTicket): add bulk_insert parameter to executemany for improved INSERT performance #463

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

feat(NoTicket): add bulk_insert parameter to executemany for improved INSERT performance #463

wants to merge 11 commits into from

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Oct 9, 2025 •
edited
Loading

Add bulk_insert parameter to executemany() for improved INSERT performance

Summary

Adds a new bulk_insert parameter to the executemany() method in both sync and async cursors. When enabled, this feature concatenates multiple INSERT statements into a single batch request with merge_prepared_statement_batches=true, providing significant performance improvements for large data insertions.

Key Changes:

  • Added bulk_insert parameter (defaults to False for backward compatibility)
  • Validates queries are INSERT statements only
  • Supports both QMARK (?) and FB_NUMERIC ($1, $2) parameter styles
  • Concatenates queries with semicolons for batch execution
  • Comprehensive unit and integration test coverage
  • Updated documentation with usage examples

Code Quality Improvements:

  • Refactored to reduce code duplication from 22.6% to 0.0% (SonarCloud)
  • Simplified validation logic using string checks instead of costly SQL parsing
  • Reuses existing query execution infrastructure where possible
  • Parametrized integration tests to eliminate duplicate code

Review & Testing Checklist for Human

Critical items requiring manual verification:

  • End-to-end database testing: Test bulk insert with real Firebolt database using both parameter styles to verify actual performance improvements and correctness
  • Parameter binding validation: Verify complex data types (dates, decimals, strings with special characters) are correctly bound in both QMARK and FB_NUMERIC modes
  • Error handling verification: Test edge cases including non-INSERT queries, empty parameter lists, malformed queries, and large datasets to ensure proper error messages and cursor state management
  • Security review: Verify query concatenation and parameter handling doesn't introduce SQL injection vulnerabilities, especially with user-provided data

Notes

  • Unit tests are comprehensive but mocked - real database integration testing is essential
  • Parameter flattening logic for FB_NUMERIC style is complex and should be tested with various data types
  • Timeout handling differs between parameter styles (FB_NUMERIC uses TimeoutController, QMARK doesn't) - verify this is intentional
  • Simple string-based INSERT validation may miss edge cases compared to full SQL parsing

Session Info: Implemented by Devin AI for @ptiurin
Session URL: https://app.devin.ai/sessions/45091a6335424e0798501799fd55a858

@devin-ai-integration @ptiurin
feat: add bulk_insert parameter to executemany for improved INSERT pe…
b57a7d3 
…rformance

- Add bulk_insert flag to executemany() in both sync and async cursors
- Concatenate multiple INSERT queries with semicolons when bulk_insert=True
- Send with merge_prepared_statement_batches=true query parameter
- Support both QMARK and FB_NUMERIC parameter styles
- Add validation to ensure only INSERT statements use bulk_insert
- Add comprehensive unit tests for both sync and async cursors
- Add integration tests for V2 (sync and async)
- Add documentation with examples for both parameter styles

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration Devin.ai Integration
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@ptiurin ptiurin changed the title feat: add bulk_insert parameter to executemany for improved INSERT performance feat(NoTicket): add bulk_insert parameter to executemany for improved INSERT performance Oct 9, 2025
devin-ai-integration bot and others added 5 commits October 9, 2025 10:03
@devin-ai-integration @ptiurin
fix: use sqlparse get_type() for reliable INSERT statement validation
ec8218c 
Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: use regex URL matching in bulk_insert tests to handle query para…
ad112ba 
…meters

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: convert URL objects to strings before split in bulk_insert tests
927aa9b 
Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: add meta field to bulk_insert test mock responses
cbbbe2e 
Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
fix: add proper statistics structure to bulk_insert test mock responses
3e898e2 
Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
ptiurin
ptiurin reviewed Oct 10, 2025
View reviewed changes
devin-ai-integration bot and others added 3 commits October 10, 2025 08:55
@devin-ai-integration @ptiurin
refactor: simplify bulk_insert validation and reuse existing executio…
2b5bb71 
…n logic

- Replace sqlparse with simple string checks in _validate_bulk_insert_query
- Add extra_params parameter to _build_fb_numeric_query_params for extensibility
- Refactor _executemany_bulk_insert to preprocess and delegate to existing methods
- Use TimeoutController for consistent timeout handling
- Parametrize integration tests to avoid duplication

Addresses PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@devin-ai-integration @ptiurin
refactor: move imports to top of integration test files
36c9f0a 
- Move firebolt.db and firebolt.async_db imports to top of test files
- Remove local import statements from inside test functions
- Use full module names instead of local aliases

Addresses final PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@ptiurin
only support this in fb_numeric
89b41a1
ptiurin
ptiurin reviewed Oct 13, 2025
View reviewed changes
docsrc/Connecting_and_queries.rst Outdated
**Note:** The ``bulk_insert`` parameter only works with INSERT statements. Using it with other
statement types (SELECT, UPDATE, DELETE, etc.) will raise an error.

Example with QMARK parameter style::
Copy link
Contributor

Choose a reason for hiding this comment

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

bulk insert with QMARK is not supported, correct this section

ptiurin and others added 2 commits October 13, 2025 17:40
@ptiurin
refactor for complexity
d13d20e
@devin-ai-integration @ptiurin
docs: clarify bulk_insert only supports fb_numeric parameter style
49854e4 
- Remove QMARK parameter style example from bulk_insert documentation
- Update note to explicitly state bulk_insert requires fb_numeric
- Clarify that using other parameter styles will raise an error

Addresses PR feedback from ptiurin on #463

Co-Authored-By: petro.tiurin@firebolt.io <petro.tiurin@firebolt.io>
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ptiurin ptiurin ptiurin left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ptiurin