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

Use usize to represent Limit::skip #3369

Closed
HaoYang670 opened this issue Sep 6, 2022 · 2 comments · Fixed by #3374
Closed

Use usize to represent Limit::skip #3369

HaoYang670 opened this issue Sep 6, 2022 · 2 comments · Fixed by #3374
Labels
enhancement New feature or request

Comments

@HaoYang670
Copy link
Contributor

HaoYang670 commented Sep 6, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

pub struct Limit {
    /// Number of rows to skip before fetch
    pub skip: Option<usize>,
    /// Maximum number of rows to fetch
    pub fetch: Option<usize>,
    /// The logical plan
    pub input: Arc<LogicalPlan>,
}

Since skip = 0 and skip = None have same meaning, we can use pure usize to represent skip to reduce complexity.

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Find this will work on #3355

@HaoYang670 HaoYang670 added the enhancement New feature or request label Sep 6, 2022
@HaoYang670
Copy link
Contributor Author

HaoYang670 commented Sep 6, 2022

cc @alamb @andygrove @Dandandan .
Need your reviews, because this will cause some public API changes.

The reasons I want to do this are:

  1. skip = None and skip = Some(0) are equivalent (forall plan, replacing skip = None with skip = 0 doesn't affect the result), so we don't need both of them.
  2. This can avoid bugs in corner cases. While working on limit_push_down, I find there are lots of 'map_or, unwrap_or, ' and pattern matching with return, which is difficult to read and easy to have bug
  3. We don't need to test skip=None, just to test skip=0 is enough.
  4. Actually, I was a little confused about the meaning of skip = None, and fetch = None. And I misunderstood that skip = None meant skipping all rows (because fetch = None means fetching all rows). I am not sure if other users/developers have ever had this confusion.

@HaoYang670 HaoYang670 changed the title Use usize to represent LogicalPlan::Limit::skip Use usize to represent Limit::skip Sep 6, 2022
@alamb
Copy link
Contributor

alamb commented Sep 7, 2022

I think this proposal makes sense to me as well. I can't think of any issues with it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants