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(questdb-driver): schema query error due to system tables #4762

Merged
merged 2 commits into from Jun 22, 2022

Conversation

puzpuzpuz
Copy link
Contributor

Check List

  • 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

#4761

@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Jun 15, 2022
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #4762 (320bdfd) into master (fc129d4) will decrease coverage by 13.31%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #4762       +/-   ##
===========================================
- Coverage   72.46%   59.15%   -13.32%     
===========================================
  Files         254      136      -118     
  Lines       27537    11186    -16351     
  Branches     2774     2781        +7     
===========================================
- Hits        19955     6617    -13338     
+ Misses       7076     4064     -3012     
+ Partials      506      505        -1     
Flag Coverage Δ
cube-backend 59.15% <ø> (-0.05%) ⬇️
cubesql ?

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

Impacted Files Coverage Δ
packages/cubejs-server-core/src/core/server.ts 64.72% <0.00%> (-1.37%) ⬇️
...nt/src/models/v1_load_request_query_filter_base.rs
...sql/src/compile/engine/information_schema/utils.rs
rust/cubesql/cubesql/src/config/injection.rs
rust/cubesql/cubeclient/src/models/v1_cube_meta.rs
...ompile/engine/information_schema/postgres/pg_am.rs
rust/cubesql/cubesql/src/sql/auth_service.rs
...e/engine/information_schema/postgres/pg_attrdef.rs
rust/cubesql/cubesql/src/sql/statement.rs
rust/cubesql/cubesql/e2e/tests/postgres.rs
... and 110 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc129d4...320bdfd. Read the comment docs.

@puzpuzpuz puzpuzpuz marked this pull request as ready for review June 15, 2022 07:32
@puzpuzpuz puzpuzpuz requested review from a team as code owners June 15, 2022 07:32
@puzpuzpuz puzpuzpuz requested a review from a team as a code owner June 15, 2022 11:40
@rpaik
Copy link
Contributor

rpaik commented Jun 15, 2022

@cristipp can you help with the review?

test('schema', async () => {
const schema = await driver.tablesSchema();

expect(schema['']).toEqual({
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are the tables query_test, telemetry and telemetry_config created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

query_test comes from the test('query', ...) test. telemetry and telemetry_config are built-in tables created for the purposes of telemetry events: https://questdb.io/docs/reference/configuration/#telemetry

@@ -3,7 +3,7 @@
exports[`questdb query measure: query 1`] = `
Array [
Object {
"Orders.totalAmount": "114",
"Orders.totalAmount": "1700",
Copy link
Contributor

Choose a reason for hiding this comment

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

Somewhat nervous, this test snapshot changed without an obvious corresponding change in the test spec. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change has to do with QuestDB version upgrade in integration tests. There was a bug in the query engine around aggregated over UNION ALL which was fixed recently. If you check packages/cubejs-testing/birdbox-fixtures/materialize/schema/Orders.js, you'll notice that 1700 is the correct value.

@puzpuzpuz
Copy link
Contributor Author

Pushed a minor typo fix in 320bdfd

@puzpuzpuz puzpuzpuz requested a review from cristipp June 22, 2022 15:56
@puzpuzpuz
Copy link
Contributor Author

@cristipp could you check the updates? Would be nice to get this PR merged, so that the fix is included into the next Cube release.

@cristipp
Copy link
Contributor

Thanks for the contribution!

@cristipp cristipp merged commit 5571a70 into cube-js:master Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
driver:questdb pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants