Skip to content

Conversation

@xinghuayu007
Copy link
Contributor

Proposed changes

select day from test where day='2020-10-32', table 'test' is parititioned by day. In this case, '2020-10-32' will be taken as CastExpr not LiteralExpr, and condition "day='2020-10-32'" will not be recognized as partitionfilter. This case will scan all partitions. To avoid scall all partitions, it is better to filter invalid date value.

issue: #4755

Types of changes

What types of changes does your code introduce to Doris?
Put an x in the boxes that apply

  • [] Bugfix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [] Documentation Update (if none of the other choices apply)
  • [] Code refactor (Modify the code structure, format the code, etc...)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have create an issue on (Fix #ISSUE), and have described the bug/feature there in detail
  • Compiling and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • If this change need a document change, I have updated the document
  • Any dependent changes have been merged

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...

rewrittenExpr = applyRuleRepeatedly(rewrittenExpr, rule, analyzer);
}
} while (oldNumChanges != numChanges_);
if (expr instanceof BinaryPredicate) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not check the type in analysis state?

if (expr instanceof BinaryPredicate) {
Expr valueExpr = ((BinaryPredicate) expr).getBinding();
if(valueExpr != null && valueExpr.getType() == Type.DATETIME && valueExpr instanceof CastExpr) {
throw new AnalysisException("invalid date type :" + valueExpr.toSql());
Copy link
Member

@xy720 xy720 Oct 20, 2020

Choose a reason for hiding this comment

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

I think throwing a exception is not the best solution of this problem, may be rewrite the binary expr day = '2020-10-32' to something like contant expr BoolLiteral[false] in where statement is better. We should discuss it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think throwing a exception is not the best solution of this problem, may be rewrite the binary expr day = '2020-10-32' to something like contant expr BoolLiteral[false] in where statement is better. We should discuss it first.

ok,I will modify it;

public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
if (!(expr instanceof BinaryPredicate)) return expr;
Expr valueExpr = expr.getChild(1);
if(valueExpr.getType() == Type.DATETIME && valueExpr instanceof CastExpr) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(valueExpr.getType() == Type.DATETIME && valueExpr instanceof CastExpr) {
if(valueExpr.getType() == Type.DATETIME && valueExpr instanceof CastExpr && valueExpr.getChild(0).getType() == Type.VARCHAR) {

There is something wrong with this If statement.
For example, where day = 20201030 will rewrite to where false, which is not right.
You should only support covert StringLiteral now:
where day = '20201032' -> where false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rule of FoldConstantsRule will convert where day = 20201030 into where day='2020-10-30 00:00:00', whic is DateLiteral type.

Copy link
Member

@xy720 xy720 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@morningman morningman added area/planner Issues or PRs related to the query planner kind/fix Categorizes issue or PR as related to a bug. approved Indicates a PR has been approved by one committer. labels Nov 3, 2020
@morningman morningman merged commit c8df76a into apache:master Nov 5, 2020
xinghuayu007 added a commit to xinghuayu007/incubator-doris that referenced this pull request Nov 11, 2020
morningman pushed a commit that referenced this pull request Nov 12, 2020
#4877)

This reverts commit c8df76a.

This commit has some problem when handling predicate like:
`k1 = "2020-10-10 10:00:00.000"`

This is a valid predicate, and FE Datetime can not support milli or micro seconds, so it will treat it as invalid date time value.

So we revert it, and may find some better solution later.
@yangzhg yangzhg mentioned this pull request Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. area/planner Issues or PRs related to the query planner kind/fix Categorizes issue or PR as related to a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants