Skip to content

[CALCITE-4426] Short-circuit evaluating when comparing two "RelTraitSet"s (Jiatao Tao)#2291

Merged
amaliujia merged 1 commit intoapache:masterfrom
Aaaaaaron:CALCITE-4426
Feb 5, 2021
Merged

[CALCITE-4426] Short-circuit evaluating when comparing two "RelTraitSet"s (Jiatao Tao)#2291
amaliujia merged 1 commit intoapache:masterfrom
Aaaaaaron:CALCITE-4426

Conversation

@Aaaaaaron
Copy link
Copy Markdown
Member

No description provided.

@MalteBellmann
Copy link
Copy Markdown
Contributor

MalteBellmann commented Dec 4, 2020

Hi, 
wouldn't it be easier to read and more general to implement this short-circuit in RelTraitSet#satisfies, as it is explained in the JavaDoc that "each trait set satisfies itself"? 
With this change:

public boolean satisfies(RelTraitSet that) {
    if (this == that){
      return true;
    }

    // now the normal check...
}

@Aaaaaaron
Copy link
Copy Markdown
Member Author

Hi, 
wouldn't it be easier to read and more general to implement this short-circuit in RelTraitSet#satisfies, as it is explained in the JavaDoc that "each trait set satisfies itself"? 
With this change:

public boolean satisfies(RelTraitSet that) {
    if (this == that){
      return true;
    }

    // now the normal check...
}

Yes, it's better, thanks!

@liyafan82
Copy link
Copy Markdown
Contributor

LGTM. This is clearly a safe improvment.

@vlsi
Copy link
Copy Markdown
Contributor

vlsi commented Dec 8, 2020

@Aaaaaaron , please remove to improve the performance of RelSubset#getRelList from JIRA and from the commit message.
The commit has virtually nothing to do with RelSubset#getRelList

@Aaaaaaron
Copy link
Copy Markdown
Member Author

@Aaaaaaron , please remove to improve the performance of RelSubset#getRelList from JIRA and from the commit message.
The commit has virtually nothing to do with RelSubset#getRelList

Sure, in the first version, the change is in getRelList and after I update the code, I forgot to change the message, thanks @vlsi !

@Aaaaaaron Aaaaaaron changed the title [CALCITE-4426] Short-circuit evaluating trait set equals to improve the performance of RelSubset#getRelList (Jiatao Tao) [CALCITE-4426] Short-circuit evaluating when comparing two "RelTraitSet"s (Jiatao Tao) Dec 8, 2020
Copy link
Copy Markdown
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

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

matches can do the same as satisfies.
It is a legit change.

@Aaaaaaron
Copy link
Copy Markdown
Member Author

Aaaaaaron commented Feb 3, 2021

Hi @amaliujia, could you help please review this PR? Thanks!

@amaliujia
Copy link
Copy Markdown
Contributor

LGTM

if there is no objections, I will merge this PR.

@amaliujia amaliujia added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Feb 4, 2021
@amaliujia amaliujia merged commit d21d540 into apache:master Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants