Skip to content

[enhance](nereids): use optional for combine()#11073

Closed
jackwener wants to merge 2 commits intoapache:masterfrom
jackwener:optional
Closed

[enhance](nereids): use optional for combine()#11073
jackwener wants to merge 2 commits intoapache:masterfrom
jackwener:optional

Conversation

@jackwener
Copy link
Member

@jackwener jackwener commented Jul 21, 2022

Proposed changes

Issue Number: close #xxx

Problem Summary:

use optional for combine()

If input list is empty, combine will return true. It's strange.

Use optional to correct it.

Checklist(Required)

  1. Does it affect the original behavior: (Yes/No/I Don't know)
  2. Has unit tests been added: (Yes/No/No Need)
  3. Has document been added or modified: (Yes/No/No Need)
  4. Does it need to update dependencies: (Yes/No)
  5. Are there any changes that cannot be rolled back: (Yes/No)

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@wangshuo128
Copy link
Contributor

wangshuo128 commented Jul 22, 2022

The current implementation looks reasonable to me.
A literal true/false predicate for empty input conjunctive/disjunctive predicates is right. It has no effect on the output data of the filter when the input predicate list is empty.
And you could check if the result is boolean literal, which has not much different from using an optional result.
A boolean literal result is much better because it has the opportunity to be optimized by boolean value rewrite rules further.

@jackwener
Copy link
Member Author

jackwener commented Jul 22, 2022

I don't think so, because this function is for combine expression by use And or Or.
Empty List is an abnormal input, it's unreasonable by using LiteralTrue as result.
And LiteralTrue defeat the intent of this function. We want to combine but return a LiteralTrue, it's strange.

@jackwener jackwener force-pushed the optional branch 2 times, most recently from ea3ebdd to c0217c0 Compare July 22, 2022 16:36
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