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

Add rewrite rule: parenthesis balance #29

Open
vlsi opened this issue Feb 26, 2020 · 1 comment
Open

Add rewrite rule: parenthesis balance #29

vlsi opened this issue Feb 26, 2020 · 1 comment

Comments

@vlsi
Copy link

vlsi commented Feb 26, 2020

The parenthesis-heavy code is often hard to reason about.

For instance:

literals.add((RexLiteral) rexBuilder.makeLiteral(
    BigDecimal.ZERO, aggregateCall.getType(), false));

Even though this code might look OK, there's a readability issue.
It is not clear if BigDecimal.ZERO is an argument to .add method or .makeLiteral method.

The very same code can be reformatted as

literals.add(
    (RexLiteral) rexBuilder.makeLiteral(
        BigDecimal.ZERO, aggregateCall.getType(), false));

or as

literals.add(
    (RexLiteral) rexBuilder.makeLiteral(
        BigDecimal.ZERO,
        aggregateCall.getType(),
        false));

Both variations make it clear that .add takes a single argument.
The observation is that the code should open at most one parenthesis on a single line.

Here's a regexp-driven implementation, however, what it does it finds a line which opens more than one unclosed parenthesis (e.g. eq(gt(x,), but it can't re-format the full expression with new indentation.

Note: eq(gt(x,2),3) is not an issue because all the open parenthesis are closed on the same line


Other samples of undesired code (see autostyle/autostyle#6 ):

    if (!(group.isRows || (group.upperBound.isUnbounded()
        && group.lowerBound.isUnbounded()))) {
    builder.addRuleCollection(ImmutableList.of((RelOptRule) SubQueryRemoveRule.FILTER,
        SubQueryRemoveRule.PROJECT,
        SubQueryRemoveRule.JOIN));
      if (((nlsString.getCharset() != null
          && type.getCharset().equals(nlsString.getCharset()))
          || (nlsString.getCharset() == null
          && SqlCollation.IMPLICIT.getCharset().equals(type.getCharset())))
          && nlsString.getCollation().equals(type.getCollation())
          && ((NlsString) value).getValue().length() == type.getPrecision()) {
        includeType = RexDigestIncludeType.NO_TYPE;
      } else {
        includeType = RexDigestIncludeType.ALWAYS;
      }
    if ((leftRowCount != null)
        && (rightRowCount != null)
        && ((leftRowCount < rightRowCount)
        || ((Math.abs(leftRowCount - rightRowCount)
        < RelOptUtil.EPSILON)
        && (rowWidthCost(left.getJoinTree())
        < rowWidthCost(right.getJoinTree()))))) {
      swap = true;
    }
    Object expr4 =
        gt(
            qw(42, 43), eq(gt(x,
                y), 2));

    Object expr = gt(eq(x, 2+(3)
      ), 5);

    Object expr2 = "(" + gt(eq(x, (3)+2
      ), 6);

    Object expr3 =
        gt(
            qw(), eq(gt(x,
                y), 2));
@jkschneider
Copy link
Collaborator

Great, thanks for all the detail @vlsi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants