Skip to content

Conversation

@aykut-bozkurt
Copy link
Member

It might be confusing for some users that hooks are still running (since at shared_preload_libraries) even if the extension is dropped. Hence, we decided to process parquet copy hooks only when pg_parquet extension is created.

@aykut-bozkurt aykut-bozkurt added the api-change includes breaking changes label Dec 16, 2024
Copy link
Collaborator

@marcoslot marcoslot left a comment

Choose a reason for hiding this comment

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

(nvm old comment, needed to redo shared_preload_libraries)


pub(crate) fn is_copy_from_parquet_stmt(p_stmt: &PgBox<PlannedStmt>) -> bool {
// the GUC pg_parquet.enable_copy_hook must be set to true
if !ENABLE_PARQUET_COPY_HOOK.get() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks, this looks a bit cleaner


// extension checks are done via catalog (not yet searched via cache by postgres till pg18)
// this is why we check them after the uri checks
extension_exists("pg_parquet") && !extension_exists("crunchy_query_engine")
Copy link
Collaborator

@marcoslot marcoslot Dec 16, 2024

Choose a reason for hiding this comment

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

thinking out loud: maybe we return silently if crunchy_query_engine exists, but throw a warning/notice if pg_parquet does not exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense

Base automatically changed from aykut/aws-url-endpoint to main December 16, 2024 13:45

if !copy_stmt.is_from {
// crunchy_query_engine should not be created
if extension_exists("crunchy_query_engine") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if this is the case, we want pg_parquet to be silent (should come before)

@codecov
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 6 lines in your changes missing coverage. Please review.

Project coverage is 92.11%. Comparing base (bab2208) to head (199c832).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/parquet_copy_hook/copy_utils.rs 79.31% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #85      +/-   ##
==========================================
- Coverage   92.18%   92.11%   -0.08%     
==========================================
  Files          71       71              
  Lines        9087     9104      +17     
==========================================
+ Hits         8377     8386       +9     
- Misses        710      718       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aykut-bozkurt aykut-bozkurt merged commit acfd42c into main Dec 16, 2024
4 of 6 checks passed
@aykut-bozkurt aykut-bozkurt deleted the aykut/check-extension branch December 16, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api-change includes breaking changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants