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.

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

feat(cubesql) Implement format / col_description #8947

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(cubesql) Implement format / col_description #8947

wants to merge 4 commits into from

Conversation

pauldheinrichs
Copy link

@pauldheinrichs pauldheinrichs commented Nov 12, 2024 •
edited
Loading

Semi-resolves: #8926

Hoping to unblock metabase failing field sync on it's latest version. This pull requests implements

  • format macro

It also noops

  • col_description
  • RegClass (for schema.table identifiers)

I don't have a tonne of bandwidth to fully implement the missing macro but this at least helps unblock the current request metabase is making to cube and failing when attempting to sync-fields.

It's not entirely clear if there's already an inherent way to lookup the table OID to get that result from the RegClass result to be leveraged in col_description implementation

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves

[For example #12]

Description of Changes Made (if issue reference is not provided)

[Description goes here]

@pauldheinrichs pauldheinrichs requested a review from a team as a code owner November 12, 2024 18:15
@vercel Vercel
Copy link

vercel bot commented Nov 12, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

8 Skipped Deployments
Name Status Preview Comments Updated (UTC)
examples-angular-dashboard ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:12pm
examples-react-d3 ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:12pm
examples-react-dashboard ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:12pm
examples-react-data-table ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:12pm
examples-react-highcharts ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:12pm
examples-react-material-ui ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:12pm
examples-react-pivot-table ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:12pm
examples-vue-query-builder ⬜️ Ignored (Inspect) Visit Preview Nov 18, 2024 4:12pm

@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Nov 12, 2024
@pauldheinrichs
Copy link
Author

pauldheinrichs commented Nov 13, 2024 •
edited
Loading 

2024-11-13 13:29:54,605 ERROR [Cube SQL Error] {"error": "Error during processing PostgreSQL message: CubeError: error in reqwest: builder error", "appName": "NULL", "protocol": "postgres", "apiType": "sql"}
thread 'main' panicked at cubesql/e2e/tests/postgres.rs:316:25:
unexpected return, simple query must be cancelled, actual: Ok(())

doesn't appear related to my changes, all specs i've added have passed 🤔

@droidraja
Copy link

droidraja commented Nov 15, 2024 •
edited
Loading 

2024-11-13 13:29:54,605 ERROR [Cube SQL Error] {"error": "Error during processing PostgreSQL message: CubeError: error in reqwest: builder error", "appName": "NULL", "protocol": "postgres", "apiType": "sql"}
thread 'main' panicked at cubesql/e2e/tests/postgres.rs:316:25:
unexpected return, simple query must be cancelled, actual: Ok(())

doesn't appear related to my changes, all specs i've added have passed 🤔

@pauldheinrichs My PR is also failing with the same set of tests. I was trying to find a PR for which this GHA was succeeding.
#8902
Will be watching this thread for any updates.

@ovr ovr mentioned this pull request Nov 18, 2024
Merged
@ovr
Copy link
Member

ovr commented Nov 18, 2024

I am not sure, but I prepared a potential fix for the CI issue: b75a37a

@pauldheinrichs Could you please rebase your PR? Thanks!

@ovr ovr self-assigned this Nov 18, 2024
@pauldheinrichs
Copy link
Author

pauldheinrichs commented Nov 18, 2024 •
edited
Loading

I am not sure, but I prepared a potential fix for the CI issue: b75a37a

@pauldheinrichs Could you please rebase your PR? Thanks!

@ovr All set! Rebased!

@codecov Codecov
Copy link

codecov bot commented Nov 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 86.95652% with 33 lines in your changes missing coverage. Please review.

Project coverage is 79.76%. Comparing base (b75a37a) to head (99db541).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...t/cubesql/cubesql/src/compile/engine/udf/common.rs 73.80% 33 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8947      +/-   ##
==========================================
- Coverage   82.66%   79.76%   -2.90%     
==========================================
  Files         221      221              
  Lines       78303    78545     +242     
==========================================
- Hits        64727    62654    -2073     
- Misses      13576    15891    +2315
Flag Coverage Δ
cubesql 79.76% <86.95%> (-2.90%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚨 Try these New Features:

@pauldheinrichs
Copy link
Author

@ovr Is there anything further you'd like to see in this PR? I'd love to merge this so we can update our metabase instance

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

@ovr ovr

@mcheshkov mcheshkov

Labels
pr:community Contribution from Cube.js community members.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Metabase] Unable to sync fields on latest version on metabase v0.51.2
4 participants
@pauldheinrichs @droidraja @ovr @mcheshkov