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

feat: add reserved column #1620

Merged
merged 3 commits into from
Dec 27, 2024
Merged

Conversation

jiacai2050
Copy link
Contributor

Rationale

In order to maintain compatibility, add a reserved column for future use.

Detailed Changes

Test Plan

CI

@jiacai2050 jiacai2050 requested a review from baojinri December 26, 2024 10:13
@github-actions github-actions bot added the feature New feature or request label Dec 26, 2024
@jiacai2050 jiacai2050 force-pushed the feat-reserved-columns branch from e4e772c to aaa5e4e Compare December 26, 2024 10:15
@jiacai2050 jiacai2050 force-pushed the feat-reserved-columns branch from aaa5e4e to 9817e45 Compare December 26, 2024 10:20
@jiacai2050 jiacai2050 requested a review from zealchen December 26, 2024 12:42
pub fn fill_builtin_projections(&self, projection: &mut Option<Vec<usize>>) {
if let Some(proj) = projection.as_mut() {
for i in 0..self.num_primary_keys {
if !proj.contains(&i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

primary keys are not builtin columns, why put them in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Primary keys column are required when query, I will rename this function to fill_required_projections

@jiacai2050 jiacai2050 requested a review from zealchen December 27, 2024 06:12
@jiacai2050 jiacai2050 force-pushed the feat-reserved-columns branch from 7a9d163 to cf2ff8a Compare December 27, 2024 06:19
@zealchen
Copy link
Contributor

LGTM

Copy link
Contributor

@zealchen zealchen left a comment

Choose a reason for hiding this comment

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

LGTM

@zealchen zealchen merged commit a8428d2 into apache:main Dec 27, 2024
6 checks passed
@jiacai2050 jiacai2050 deleted the feat-reserved-columns branch December 27, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants