You are viewing a plain text version of this content. The canonical link for it is here.
Posted to devnull@infra.apache.org by "sachouche (GitHub)" <gi...@apache.org> on 2018/08/24 15:59:50 UTC
[GitHub] [drill] sachouche opened pull request #1444: DRILL-6709:
Extended the batch stats utility to other operators
- Enhanced the Batch Stats utility to handle input / output operators as it was being used only by the Parquet reader
- Modified all batch stats logging (from other operators which support batch sizing) to use the Batch Sizing utility
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] sachouche commented on pull request #1444:
DRILL-6709: Extended the batch stats utility to other operators
Posted by "sachouche (GitHub)" <gi...@apache.org>.
Renamed the enum to RecordBatchIOType; I want to include the IO keyword to avoid conflating this enum type with data type.
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] sachouche commented on pull request #1444:
DRILL-6709: Extended the batch stats utility to other operators
Posted by "sachouche (GitHub)" <gi...@apache.org>.
I have seen both types of indentation being used; I have implemented your suggestion.
and yes, I am aware that Java enums are more powerful; for now didn't have a need to enhance it. I have a added a private variable IOTypeString in case caller wants to get the string representation. Didn't use this in the code as I wanted to avoid expensive string concatenation (especially now that the caller is not using the isStatsLoggingEnabled() check).
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] bitblender commented on pull request #1444:
DRILL-6709: Extended the batch stats utility to other operators
Posted by "bitblender (GitHub)" <gi...@apache.org>.
This outer check is unnecessary because logRecordBatchStats does the same check. I guess you have it here just to avoid the cost of String.format(). Maybe we should add a var args version which does the string.format internally? When reading code, I personally like it when I can read as much of the real function logic in one page, large bodies of logging code make this harder. I realize it is just 2 extra lines per logging location but it makes it more verbose.
That said, if you are using an outer check, it allows you to use the "+" string builder and not use String.format(). String.format() is prone to mismatch errors between the order in the format string and the order in the variable list.
This is more of a style comment. I will be ok if you don't change it now.
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] sachouche commented on pull request #1444:
DRILL-6709: Extended the batch stats utility to other operators
Posted by "sachouche (GitHub)" <gi...@apache.org>.
Made the changes.
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] sachouche commented on pull request #1444:
DRILL-6709: Extended the batch stats utility to other operators
Posted by "sachouche (GitHub)" <gi...@apache.org>.
I agree with your feedback; I was anyway going to make this improvement as I didn't like the extra condition. I added another method which takes a string format and variable arguments; modified most of the previous calls to use this method.
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] bitblender commented on pull request #1444:
DRILL-6709: Extended the batch stats utility to other operators
Posted by "bitblender (GitHub)" <gi...@apache.org>.
'case' is generally indented one level into the 'switch'.
Also, Java allows Enums to have constructors and methods that can be used to associate a string with an enum value. This makes modification of enum names and attributes easier. See https://docs.oracle.com/javase/tutorial/java/javaOO/enum.html
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] sachouche commented on issue #1444: DRILL-6709:
Extended the batch stats utility to other operators
Posted by "sachouche (GitHub)" <gi...@apache.org>.
Thank you @bitblender and @ilooner for performing the review!
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] sachouche commented on issue #1444: DRILL-6709:
Extended the batch stats utility to other operators
Posted by "sachouche (GitHub)" <gi...@apache.org>.
@ilooner and @bitblender, can you please review this PR?
Thanks!
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] Ben-Zvi closed pull request #1444: DRILL-6709:
Extended the batch stats utility to other operators
Posted by "Ben-Zvi (GitHub)" <gi...@apache.org>.
[ pull request closed by Ben-Zvi ]
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] sachouche commented on issue #1444: DRILL-6709:
Extended the batch stats utility to other operators
Posted by "sachouche (GitHub)" <gi...@apache.org>.
@ilooner can you please review this PR?
thanks!
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] sachouche commented on issue #1444: DRILL-6709:
Extended the batch stats utility to other operators
Posted by "sachouche (GitHub)" <gi...@apache.org>.
@bitblender thank you for your review! I have implemented all the feedback that you requested.
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] bitblender commented on pull request #1444:
DRILL-6709: Extended the batch stats utility to other operators
Posted by "bitblender (GitHub)" <gi...@apache.org>.
Please remove the 'IS_' prefix in the enum values. isXXX is typically used for functions that return a boolean value.
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [drill] bitblender commented on pull request #1444:
DRILL-6709: Extended the batch stats utility to other operators
Posted by "bitblender (GitHub)" <gi...@apache.org>.
I feel "RecordBatchType" describes the enum better then "BatchIOType"
[ Full content available at: https://github.com/apache/drill/pull/1444 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org