[GitHub] [incubator-druid] suneet-amp commented on a change in pull request #8822: optimize numeric column null value checking for low filter selectivity (more rows)
suneet-amp commented on a change in pull request #8822: optimize numeric column
null value checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#discussion_r345466852
##
File path:
processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java
##
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.collections.bitmap;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.IntIterator;
+import org.roaringbitmap.PeekableIntIterator;
+
+public class PeekableIteratorAdapter
implements PeekableIntIterator
+{
+ static final int NOT_SET = -1;
+ final TIntIterator baseIterator;
+ int mark = NOT_SET;
+
+ PeekableIteratorAdapter(TIntIterator iterator)
+ {
+this.baseIterator = Preconditions.checkNotNull(iterator, "iterator");
+ }
+
+ @Override
+ public void advanceIfNeeded(int i)
+ {
+while (mark < i && baseIterator.hasNext()) {
Review comment:
sgtm
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
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] suneet-amp commented on a change in pull request #8822: optimize numeric column null value checking for low filter selectivity (more rows)
suneet-amp commented on a change in pull request #8822: optimize numeric column
null value checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#discussion_r344947786
##
File path:
processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java
##
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.collections.bitmap;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.IntIterator;
+import org.roaringbitmap.PeekableIntIterator;
+
+public class PeekableIteratorAdapter
implements PeekableIntIterator
+{
+ static final int NOT_SET = -1;
+ final TIntIterator baseIterator;
+ int mark = NOT_SET;
+
+ PeekableIteratorAdapter(TIntIterator iterator)
+ {
+this.baseIterator = Preconditions.checkNotNull(iterator, "iterator");
+ }
+
+ @Override
+ public void advanceIfNeeded(int i)
+ {
+while (mark < i && baseIterator.hasNext()) {
Review comment:
Thinking out loud here - feel free to ignore.
According to the interface javadocs `i` is the `minVal` that we want to
advance to. There is no guarantee that minVal will always be positive. It looks
like in right now we only pass in a positive integer, but it's possible someone
can pass in negative values in the future? (if the baseIterator is a list of
sorted integers - not sure if this is ever the case in druid)
If we set `mark` to Integer.MIN_VALUE instead of -1 will that support
negative numbers as well without a performance hit?
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
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] suneet-amp commented on a change in pull request #8822: optimize numeric column null value checking for low filter selectivity (more rows)
suneet-amp commented on a change in pull request #8822: optimize numeric column
null value checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#discussion_r344932597
##
File path:
processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java
##
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.collections.bitmap;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.IntIterator;
+import org.roaringbitmap.PeekableIntIterator;
+
+public class PeekableIteratorAdapter
implements PeekableIntIterator
+{
+ static final int NOT_SET = -1;
+ final TIntIterator baseIterator;
+ int mark = NOT_SET;
Review comment:
Either is fine, maybe a javadoc explaining what it is.
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
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] suneet-amp commented on a change in pull request #8822: optimize numeric column null value checking for low filter selectivity (more rows)
suneet-amp commented on a change in pull request #8822: optimize numeric column
null value checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#discussion_r344426269
##
File path:
processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java
##
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.collections.bitmap;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.IntIterator;
+import org.roaringbitmap.PeekableIntIterator;
+
+public class PeekableIteratorAdapter
implements PeekableIntIterator
+{
+ static final int NOT_SET = -1;
+ final TIntIterator baseIterator;
Review comment:
Do you really want all of these to be package private instead of protected?
also nit: empty line between static and class variables
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
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] suneet-amp commented on a change in pull request #8822: optimize numeric column null value checking for low filter selectivity (more rows)
suneet-amp commented on a change in pull request #8822: optimize numeric column
null value checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#discussion_r344426863
##
File path:
processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java
##
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.collections.bitmap;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.IntIterator;
+import org.roaringbitmap.PeekableIntIterator;
+
+public class PeekableIteratorAdapter
implements PeekableIntIterator
+{
+ static final int NOT_SET = -1;
+ final TIntIterator baseIterator;
+ int mark = NOT_SET;
+
+ PeekableIteratorAdapter(TIntIterator iterator)
+ {
+this.baseIterator = Preconditions.checkNotNull(iterator, "iterator");
+ }
+
+ @Override
+ public void advanceIfNeeded(int i)
+ {
+while (mark < i && baseIterator.hasNext()) {
Review comment:
is `i` always guaranteed to be positive?
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
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] suneet-amp commented on a change in pull request #8822: optimize numeric column null value checking for low filter selectivity (more rows)
suneet-amp commented on a change in pull request #8822: optimize numeric column
null value checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#discussion_r344427395
##
File path:
processing/src/main/java/org/apache/druid/collections/bitmap/ImmutableBitmap.java
##
@@ -33,6 +34,14 @@
*/
IntIterator iterator();
+ /**
+ * @return a peekable iterator which can skip to a position
+ */
+ default PeekableIntIterator peekableIterator()
+ {
+return new PeekableIteratorAdapter(iterator());
Review comment:
nit: IntelliJ is complaining about raw types here
`return new PeekableIteratorAdapter<>(iterator());`
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
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
[GitHub] [incubator-druid] suneet-amp commented on a change in pull request #8822: optimize numeric column null value checking for low filter selectivity (more rows)
suneet-amp commented on a change in pull request #8822: optimize numeric column
null value checking for low filter selectivity (more rows)
URL: https://github.com/apache/incubator-druid/pull/8822#discussion_r344427249
##
File path:
processing/src/main/java/org/apache/druid/collections/bitmap/PeekableIteratorAdapter.java
##
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.collections.bitmap;
+
+import com.google.common.base.Preconditions;
+import org.roaringbitmap.IntIterator;
+import org.roaringbitmap.PeekableIntIterator;
+
+public class PeekableIteratorAdapter
implements PeekableIntIterator
+{
+ static final int NOT_SET = -1;
+ final TIntIterator baseIterator;
+ int mark = NOT_SET;
Review comment:
nit: `mark` was confusing for me to understand. Is this `nextVal`?
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
-
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
