julianhyde commented on issue #1334: [CALCITE-3111] Allow custom 
implementations of Correlate in RelDecorr…
URL: https://github.com/apache/calcite/pull/1334#issuecomment-521755452
 
 
   I'm "meh" on the javadoc for all the `decorrelateRel` methods. The methods 
are called via reflective dispatch so they all do the same thing. Adding 
javadoc doesn't add any information content.
   
   Don't put ' );' on a new line. We don't format code that way.
   
   Please add commits, don't amend or squash. It's difficult to review when you 
squash each time. (Some reviewers ask for squash and rebase. I guess I disagree 
with those reviewers. I'm happy to squash and rebase when I merge your PR.)
   
   I think you need to change algebra.md. 
   
   I hate code formatted like this:
   
   ```
           RelNode distinct = relBuilder.push(newInput).projectNamed(
               positions.stream().map(i -> relBuilder.getRexBuilder()
                   .makeInputRef(newInput, i)).collect(Collectors.toList()),
               
positions.stream().map(fieldNames::get).collect(Collectors.toList()),
               false)
                .distinct()
                .build();
   ```
   
   Breaking the line at the line length limit seems to me like 
passive-aggressive adherence to the line length policy. Isn't this more 
readable?
   
   ```
           RelNode distinct = relBuilder.push(newInput)
               .projectNamed(positions.stream()
                       .map(i ->
                           relBuilder.getRexBuilder().makeInputRef(newInput, i))
                       .collect(Collectors.toList()),
                   
positions.stream().map(fieldNames::get).collect(Collectors.toList()),
                   false)
               .distinct()
               .build();
   ```
   
   That said, wouldn't 
   ```
           RelNode distinct = relBuilder.push(newInput)
               .project(relBuilder.fields(positions))
               .distinct()
               .build();
   ```
   do the same thing?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to