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

refactor(relational_states): use fe table catalog for hash agg #3484

Merged
merged 7 commits into from
Jun 27, 2022

Conversation

BowenXiao1999
Copy link
Contributor

@BowenXiao1999 BowenXiao1999 commented Jun 27, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

  • Fix the wrong infer in infer_internal_table_catalog. Done some Simple DRY. Previous code is hard to maintain and read... May need more work in future.
  • CN parse the table catalog and init the state table.
  • Tweak some unit test, infer table catalog manually.

Part of #3475

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Jun 27, 2022

Codecov Report

Merging #3484 (78f9452) into main (744f71c) will decrease coverage by 0.06%.
The diff coverage is 41.25%.

❗ Current head 78f9452 differs from pull request most recent head 7081bc5. Consider uploading reports for the commit 7081bc5 to get more accurate results

@@            Coverage Diff             @@
##             main    #3484      +/-   ##
==========================================
- Coverage   74.43%   74.36%   -0.07%     
==========================================
  Files         769      769              
  Lines      107554   107397     -157     
==========================================
- Hits        80060    79870     -190     
- Misses      27494    27527      +33     
Flag Coverage Δ
rust 74.36% <41.25%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
src/stream/src/executor/aggregation/mod.rs 93.18% <ø> (ø)
src/stream/src/from_proto/hash_agg.rs 0.00% <0.00%> (ø)
...rc/frontend/src/optimizer/plan_node/logical_agg.rs 92.92% <48.80%> (-3.49%) ⬇️
src/stream/src/executor/hash_agg.rs 97.82% <100.00%> (+0.01%) ⬆️
src/frontend/src/handler/handle_privilege.rs 65.97% <0.00%> (-1.36%) ⬇️
src/batch/src/executor/row_seq_scan.rs 15.47% <0.00%> (-1.09%) ⬇️
src/object_store/src/object/mem.rs 84.84% <0.00%> (-1.02%) ⬇️
src/common/src/cache.rs 96.73% <0.00%> (-0.85%) ⬇️
...ge/src/hummock/sstable/forward_sstable_iterator.rs 95.12% <0.00%> (-0.72%) ⬇️
src/storage/src/hummock/iterator/backward_user.rs 95.82% <0.00%> (-0.70%) ⬇️
... and 25 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@BowenXiao1999 BowenXiao1999 force-pushed the bw/use_fe_table_catalog_for_hash_agg branch from e1f1a88 to 78f9452 Compare June 27, 2022 09:41
Copy link
Contributor

@st1page st1page left a comment

Choose a reason for hiding this comment

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

LGTM

src/stream/src/from_proto/hash_agg.rs Outdated Show resolved Hide resolved
@BowenXiao1999 BowenXiao1999 added the mergify/can-merge Indicates that the PR can be added to the merge queue label Jun 27, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2022

Hey @BowenXiao1999, this pull request failed to merge and has been dequeued from the merge train. If you believe your PR failed in the merge train because of a flaky test, requeue it by commenting with @mergifyio requeue. More details can be found on the Queue: Embarked in merge train check-run.

@BowenXiao1999 BowenXiao1999 force-pushed the bw/use_fe_table_catalog_for_hash_agg branch from 7081bc5 to c542d8d Compare June 27, 2022 11:26
@BowenXiao1999
Copy link
Contributor Author

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jun 27, 2022

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@BowenXiao1999 BowenXiao1999 added mergify/can-merge Indicates that the PR can be added to the merge queue and removed mergify/can-merge Indicates that the PR can be added to the merge queue labels Jun 27, 2022
@BowenXiao1999 BowenXiao1999 enabled auto-merge (squash) June 27, 2022 11:29
@BowenXiao1999 BowenXiao1999 merged commit 321c259 into main Jun 27, 2022
@BowenXiao1999 BowenXiao1999 deleted the bw/use_fe_table_catalog_for_hash_agg branch June 27, 2022 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergify/can-merge Indicates that the PR can be added to the merge queue type/refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants