Skip to content

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

@fanyang01 fanyang01 requested a review from Copilot January 8, 2025 10:32
Copy link

Copilot AI left a comment

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
@fanyang01
Copy link
Contributor

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

Copy link
Contributor

@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

3 participants