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

chore(relational_layer): add value_meta in relational write interfaces #2194

Merged
merged 2 commits into from
Apr 28, 2022

Conversation

wcy-fdu
Copy link
Contributor

@wcy-fdu wcy-fdu commented Apr 28, 2022

What's changed and what's your intention?

As title, relational layer provides hash and non-hash write interfaces.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #2194 (424246d) into main (9bce175) will decrease coverage by 0.02%.
The diff coverage is 44.82%.

@@            Coverage Diff             @@
##             main    #2194      +/-   ##
==========================================
- Coverage   70.92%   70.90%   -0.03%     
==========================================
  Files         650      650              
  Lines       82587    82642      +55     
==========================================
+ Hits        58578    58600      +22     
- Misses      24009    24042      +33     
Flag Coverage Δ
rust 70.90% <44.82%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
src/stream/src/executor_v2/mview/materialize.rs 91.33% <ø> (ø)
src/storage/src/table/cell_based_table.rs 79.53% <41.25%> (-10.01%) ⬇️
src/storage/src/table/state_table.rs 90.40% <85.71%> (-0.09%) ⬇️
.../src/executor/managed_state/aggregation/extreme.rs 90.02% <0.00%> (-0.27%) ⬇️
src/source/src/manager.rs 45.74% <0.00%> (ø)
src/meta/src/cluster/mod.rs 59.39% <0.00%> (ø)
src/batch/src/executor2/insert.rs 80.82% <0.00%> (ø)
src/source/src/connector_source.rs 0.00% <0.00%> (ø)
src/common/src/types/ordered_float.rs 24.30% <0.00%> (+0.19%) ⬆️
src/batch/src/executor2/delete.rs 79.50% <0.00%> (+0.47%) ⬆️
... and 3 more

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

Copy link
Contributor

@xx01cyx xx01cyx left a comment

Choose a reason for hiding this comment

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

LGTM. But I think an alternative is to have an inner method with a flag param indicating whether value meta needs computing, like:

fn batch_write_rows_inner(..., stateful_flag: bool) -> StorageResult<()> {
    ...
    if stateful_flag {
        // compute value meta
    }
}

and call this inner method in batch_write_rows and batch_write_rows_inner to make the code clearer.

@xx01cyx
Copy link
Contributor

xx01cyx commented Apr 28, 2022

But since it is a storage write API, maybe we should put efficiency first and reduce unnecessary function call and if branch...

@wcy-fdu
Copy link
Contributor Author

wcy-fdu commented Apr 28, 2022

But since it is a storage write API, maybe we should put efficiency first and reduce unnecessary function call and if branch...

I prefer reduce unnecessary if judgment, which may pass a stateful_flag while write to storage.

@wcy-fdu wcy-fdu merged commit 34e2e19 into main Apr 28, 2022
@wcy-fdu wcy-fdu deleted the wcy_add_value_meta_in_relational_layer branch April 28, 2022 08:07
@skyzh
Copy link
Contributor

skyzh commented Apr 28, 2022

Two small issues:

  • value meta isn't always pk. Should have another option to specify column ids (or column indices) of value meta.
  • batch_write_rows_with_value_meta seems to have a lot of code in common with batch_write_rows. Maybe we can do some dedups.

@skyzh
Copy link
Contributor

skyzh commented Apr 28, 2022

If you are interested, you may also refactor MaterializeExecutor to use distribution key for value meta.

#1971

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants