You are viewing a plain text version of this content. The canonical link for it is here.
Posted to devnull@infra.apache.org by "himanshug (GitHub)" <gi...@apache.org> on 2018/08/23 17:02:12 UTC
[GitHub] [incubator-druid] himanshug opened pull request #6223: make
COMPLEX column filterable in Druid code
This patch removes the assumption made by Druid core that COMPLEX columns can't support filters.
I'm writing a custom aggregator.. extension takes full control of column representation by writing custom GenericColumnSerializer, and stores bitmaps for fast filtering based on values in this column.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] himanshug commented on issue #6223: make
COMPLEX column optionally filterable in Druid code
Posted by "himanshug (GitHub)" <gi...@apache.org>.
@gianm @clintropolis alright, so I have added a new method in `ColumnCapability` . To retain original behavior all STRING, FLOAT, DOUBLE, LONG columns are always filterable , for other (aka COMPLEX) columns filterability can be explicitly set optionally.
`TeamCity` build is failing because `ColumnBuilder.setFilterable(..)` function is unused in Druid code (but used in the my extension). Does anyone know what comments I can add to exclude that method from the check?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] clintropolis commented on issue #6223:
make COMPLEX column optionally filterable in Druid code
Posted by "clintropolis (GitHub)" <gi...@apache.org>.
Hmm, I might be confusing it with intellij 😛
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm closed pull request #6223: make
COMPLEX column optionally filterable in Druid code
Posted by "gianm (GitHub)" <gi...@apache.org>.
[ pull request closed by gianm ]
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] himanshug commented on issue #6223: make
COMPLEX column filterable in Druid code
Posted by "himanshug (GitHub)" <gi...@apache.org>.
@gianm agree that columns can be filterable without bitmap indexes. however, independently,
only use of `Filter.FILTERABLE_TYPE` is in `ColumnSelectorBitmapIndexSelector.columnSupportsFiltering(column)` which looks like
```
private static boolean columnSupportsFiltering(Column column)
{
ValueType columnType = column.getCapabilities().getType();
return Filters.FILTERABLE_TYPES.contains(columnType);
}
```
and at its call sites, it is used to check whether to work with an empty bitmap or get one from column. it appears, everything would stay exactly same even if I changed above implementation to...
```
private static boolean columnSupportsFiltering(Column column)
{
return column.getCapabilities().hasBitmapIndexes();
}
```
with above done, there is no more any usage of `Filter.FILTERABLE_TYPE` .
I must be missing something.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] himanshug commented on issue #6223: make
COMPLEX column optionally filterable in Druid code
Posted by "himanshug (GitHub)" <gi...@apache.org>.
no, I think you are right ... teamcity is from jetbrains and uses same set of inspections. so I guess it would be same... but the build will confirm soon.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] clintropolis commented on issue #6223:
make COMPLEX column optionally filterable in Druid code
Posted by "clintropolis (GitHub)" <gi...@apache.org>.
`@SuppressWarnings("unused")` I believe
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] himanshug commented on issue #6223: make
COMPLEX column filterable in Druid code
Posted by "himanshug (GitHub)" <gi...@apache.org>.
> Would it make sense to add a canFilter method to ColumnCapabilities to have a finer level of control than just mapping filterability directly to ValueType?
Actually I think existence of `Filterable.FILTERABLE_TYPE` is redundant. For me, ColumnCapability.hasBitmapIndexes() is already the indication whether column supports filtering or not. So, one thing to do is to remove `Filterable.FILTERABLE_TYPE` altogether and adjust the code in `ColumnSelectorBitmapIndexSelector` to not use that.
however, I would like to keep things simple for this PR and not cause any unwanted side effects as above suggestion needs to get tested appropriately.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] himanshug commented on issue #6223: make
COMPLEX column filterable in Druid code
Posted by "himanshug (GitHub)" <gi...@apache.org>.
@gianm ignore above, I see that the behavior changes with above modification because it would now return false for LONG columns. nvrmnd .
yeah, we can introduce another method in ColumnCapability interface to replace `Filterable.FILTERABLE_TYPE` .
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on issue #6223: make
COMPLEX column filterable in Druid code
Posted by "gianm (GitHub)" <gi...@apache.org>.
> Actually I think existence of Filterable.FILTERABLE_TYPE is redundant. For me, ColumnCapability.hasBitmapIndexes() is already the indication whether column supports filtering or not.
They don't mean the same thing: for example, long columns are filterable but do not have bitmap indexes. IMO a capability makes sense and is a good way to solve this.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] gianm commented on issue #6223: make
COMPLEX column filterable in Druid code
Posted by "gianm (GitHub)" <gi...@apache.org>.
@himanshug probably it makes sense for this new capability to always be true for string, long, float, double; and for it to be false by default for complex, but it can be overridden.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] clintropolis commented on issue #6223:
make COMPLEX column filterable in Druid code
Posted by "clintropolis (GitHub)" <gi...@apache.org>.
Would it make sense to add a `canFilter` method to `ColumnCapabilities` to have a finer level of control than just mapping filterability directly to `ValueType`?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] clintropolis commented on issue #6223:
make COMPLEX column optionally filterable in Druid code
Posted by "clintropolis (GitHub)" <gi...@apache.org>.
If it's not that it's probably
```
// noinspection unused
```
?
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org
[GitHub] [incubator-druid] himanshug commented on issue #6223: make
COMPLEX column optionally filterable in Druid code
Posted by "himanshug (GitHub)" <gi...@apache.org>.
@clintropolis thanks, dint realize it was same as that for intellij... let me add that.
[ Full content available at: https://github.com/apache/incubator-druid/pull/6223 ]
This message was relayed via gitbox.apache.org for devnull@infra.apache.org