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(core)!: Return Arc<AccessInfo> for metadata #4883

Merged
merged 3 commits into from
Jul 12, 2024

Conversation

Lzzzzzt
Copy link
Contributor

@Lzzzzzt Lzzzzzt commented Jul 12, 2024

Just add a field inside the Operator struct and wrap the OperatorInfo with OnceCell<Arc<T>> for lazy initialize and avoid other memory allocation.

#4845

…metadata

Signed-off-by: Lzzzt <liuzitao0123@gmail.com>
@Lzzzzzt
Copy link
Contributor Author

Lzzzzzt commented Jul 12, 2024

One question: using Arc<T> will cause heap allocation, we can store the OperatorInfo just in the Operator without wrapping in Arc<T>, but the size of Operator will be huge(300B), which is better?

@Lzzzzzt Lzzzzzt marked this pull request as ready for review July 12, 2024 02:54
@Lzzzzzt Lzzzzzt requested a review from Xuanwo as a code owner July 12, 2024 02:54
@Xuanwo
Copy link
Member

Xuanwo commented Jul 12, 2024

Replied at #4845 (comment)

@Lzzzzzt Lzzzzzt force-pushed the operator-info branch 2 times, most recently from d6ce514 to e45455f Compare July 12, 2024 06:46
core/src/layers/blocking.rs Outdated Show resolved Hide resolved
@@ -380,13 +380,14 @@ impl<A: Access> LayeredAccess for CompleteAccessor<A> {
&self.inner
}

fn metadata(&self) -> AccessorInfo {
let mut meta = self.meta.clone();
// Todo: May move the logic to the implement of Layer::layer of CompleteAccessor<A>
Copy link
Member

Choose a reason for hiding this comment

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

Nice idea, we can create an issue for this.

Copy link
Contributor Author

@Lzzzzzt Lzzzzzt Jul 12, 2024

Choose a reason for hiding this comment

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

I think the issue can be create after this refactor is done, because this may be related to the #4845 (comment)

core/src/types/operator/operator.rs Outdated Show resolved Hide resolved
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Other LGTM, thanks!

integrations/object_store/src/store.rs Outdated Show resolved Hide resolved
@Xuanwo Xuanwo changed the title refactor: Operator add a field to store OperatorInfo refactor(core)!: Return Arc<AccessInfo> for metadata Jul 12, 2024
@Lzzzzzt Lzzzzzt force-pushed the operator-info branch 4 times, most recently from b4c9cfa to e39efee Compare July 12, 2024 10:05
Signed-off-by: Lzzzt <liuzitao0123@gmail.com>

refactor: change the signature of `Access::info` and `AccessDyn::info`

Signed-off-by: Lzzzt <liuzitao0123@gmail.com>

refactor: change the signature of `Access::info` and `AccessDyn::info`

Signed-off-by: Lzzzt <liuzitao0123@gmail.com>

revert: revert

Signed-off-by: Lzzzt <liuzitao0123@gmail.com>

revert: revert

Signed-off-by: Lzzzt <liuzitao0123@gmail.com>

Update store.rs

refactor: change the signature of `Access::info` and `AccessDyn::info`

Signed-off-by: Lzzzt <liuzitao0123@gmail.com>
@Lzzzzzt Lzzzzzt marked this pull request as ready for review July 12, 2024 10:44
@Lzzzzzt Lzzzzzt requested a review from Xuanwo July 12, 2024 10:44
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Perfect, thanks a lot!

@Xuanwo Xuanwo merged commit 9ef494d into apache:main Jul 12, 2024
219 checks passed
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.

2 participants