You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/01/16 08:31:36 UTC
[GitHub] [druid] clintropolis commented on a change in pull request #9179:
Run IntelliJ inspections on Travis
clintropolis commented on a change in pull request #9179: Run IntelliJ inspections on Travis
URL: https://github.com/apache/druid/pull/9179#discussion_r367287105
##########
File path: processing/src/main/java/org/apache/druid/segment/data/ColumnarLongs.java
##########
@@ -121,14 +122,14 @@ public boolean isNull()
@Override
public long getLong()
{
- assert NullHandling.replaceWithDefault() || !isNull();
+ Preconditions.checkState(NullHandling.replaceWithDefault() || !isNull());
Review comment:
This change to column gets should definitely not be done imo, the current version with `assert` will not be evaluated at all at runtime unless asserts are enabled with jvm command line flags, and isNull is an expensive check if the bit is set in the bitmap.
In fact, I've been meaning to investigate if they should be removed entirely because they are still in the bytecode and could affect hotspot function inlining which takes function size into account, since these methods will be called from hot loops, but I haven't had the chance to investigate.
e.g. using columnar doubles
with assert:
```
public double getDouble(int);
Code:
0: getstatic #13 // Field $assertionsDisabled:Z
3: ifne 33
6: invokestatic #14 // Method org/apache/druid/common/config/NullHandling.replaceWithDefault:()Z
9: ifne 33
12: aload_0
13: getfield #2 // Field val$nullValueBitmap:Lorg/apache/druid/collections/bitmap/ImmutableBitmap;
16: iload_1
17: invokeinterface #19, 2 // InterfaceMethod org/apache/druid/collections/bitmap/ImmutableBitmap.get:(I)Z
22: ifeq 33
25: new #16 // class java/lang/AssertionError
28: dup
29: invokespecial #17 // Method java/lang/AssertionError."<init>":()V
32: athrow
33: aload_0
34: getfield #1 // Field this$0:Lorg/apache/druid/segment/data/ColumnarDoubles;
37: iload_1
38: invokeinterface #18, 2 // InterfaceMethod org/apache/druid/segment/data/ColumnarDoubles.get:(I)D
43: dreturn
```
no assert:
```
public double getDouble(int);
Code:
0: aload_0
1: getfield #1 // Field this$0:Lorg/apache/druid/segment/data/ColumnarDoubles;
4: iload_1
5: invokeinterface #13, 2 // InterfaceMethod org/apache/druid/segment/data/ColumnarDoubles.get:(I)D
10: dreturn
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org