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: resolve compatibilities for Metabase (#340) #352

Merged
merged 10 commits into from
Jan 9, 2025
Merged

Conversation

VWagen1989
Copy link
Contributor

@VWagen1989 VWagen1989 commented Jan 8, 2025

This PR addresses the issues of the compatiblities between MyDuck and Metabase.
The other BI tools are not tested yet. If there still be compatibility issues for them, we should open another PR for them.

Ref #340
Resolve #342

pgserver/iter.go Outdated Show resolved Hide resolved
pgserver/stmt.go Outdated Show resolved Hide resolved
pgserver/stmt.go Show resolved Hide resolved
@fanyang01 fanyang01 requested a review from Copilot January 8, 2025 10:32

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 10 changed files in this pull request and generated no comments.

Files not reviewed (5)
  • pgserver/connection_data.go: Evaluated as low risk
  • pgserver/duck_handler.go: Evaluated as low risk
  • pgserver/iter.go: Evaluated as low risk
  • catalog/provider.go: Evaluated as low risk
  • pgserver/connection_handler.go: Evaluated as low risk
Comments suppressed due to low confidence (3)

pgserver/in_place_handler.go:124

  • Ensure that the new InPlaceHandler struct and its methods are covered by tests.
type InPlaceHandler struct {

pgserver/in_place_handler.go:140

  • Ensure that the selectionConversions logic is covered by tests, especially the regex patterns and query conversions.
var selectionConversions = []SelectionConversion{

pgserver/in_place_handler.go:374

  • Ensure that the updated shouldQueryBeHandledInPlace function is covered by tests, including all its call sites.
func shouldQueryBeHandledInPlace(h *ConnectionHandler, sql *ConvertedStatement) (bool, error) {
@VWagen1989 VWagen1989 enabled auto-merge (squash) January 9, 2025 08:53
pgserver/connection_data.go Show resolved Hide resolved
pgserver/in_place_handler.go Outdated Show resolved Hide resolved
pgserver/in_place_handler.go Outdated Show resolved Hide resolved
@fanyang01
Copy link
Collaborator

The failed COPY DATABASE test will be auto-fixed in DuckDB 1.2: duckdb/duckdb#15548

Copy link
Collaborator

@fanyang01 fanyang01 left a comment

Choose a reason for hiding this comment

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

Thanks! Look good. It's the first BI tool that we can seamlessly integrate with. Congratulations!

@VWagen1989 VWagen1989 merged commit 34b0213 into main Jan 9, 2025
14 checks passed
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.

Incorrect response packet for Describe command while handling extended protocol requests
2 participants