-
Notifications
You must be signed in to change notification settings - Fork 173
feat(go/adbc/driver/snowflake): Add option to disable vectorized scanner #3555
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(go/adbc/driver/snowflake): Add option to disable vectorized scanner #3555
Conversation
| OptionStatementIngestTargetFileSize = "adbc.snowflake.statement.ingest_target_file_size" | ||
| OptionStatementIngestCompressionCodec = "adbc.snowflake.statement.ingest_compression_codec" // TODO(GH-1473): Implement option | ||
| OptionStatementIngestCompressionLevel = "adbc.snowflake.statement.ingest_compression_level" // TODO(GH-1473): Implement option | ||
| OptionStatementVectorizedScanner = "adbc.snowflake.statement.use_vectorized_scanner" |
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.
Hmm... should probably reference ingestion. Maybe ingest_use_vectorized_scanner?
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.
Sounds reasonable to me.
(Also not quite an issue for here but the adbc. prefix is probably redundant)
| }) | ||
|
|
||
| // Create a temporary stage, we can't start uploading until it has been created | ||
| createTemporaryStageStmt := fmt.Sprintf(createTemporaryStageTmpl, "") |
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.
Why don't we handle the option here?
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.
My read of the code is that this only ever binds a single record so I don't know why you'd ever need the vectorized scanner.
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.
It could bind a single very large record :)
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.
Added this for consistency and compatibility, though I'm skeptical that there's any real-world value ;).
The vectorized scanner seems to be causing performance issues under some circumstances. Add an option to disable it.