-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
should we deprecate fieldExpression and extractionFn field in various Aggregators, DimensionSpecs, Filters etc #8242
Comments
sgtm 👍 |
I’m a bit nervous about removing something so widely used. I wonder if we can introduce a compat shim that translates these kinds of filters? |
@gianm I think we could do something on the line of rewriting the query in early stage of processing pipeline at broker to... for each aggregator/filter/whatever, auto generate the virtual columns spec and set extractionFn field to null in it. that said, doing and testing it is overhead if we decide to "deprecate and remove" it. OTOH if you mean that we should not deprecate extractionFn field and will always keep the compatibility layer, then it may make sense to write it. |
IMO if the deprecation cycle is long-ish (multiple release cycles) and generally aligned with moving most users to SQL then it makes sense to me. The thoughts guiding me here are:
We could consider getting rid of the |
I agree with all observations you made and road to "remove" will be a very long one as plenty of time needs to be given before they are removed. main motivator for me really is to simplify the implementation of filters and aggregators etc. I don't have anything against the API except limiting the ways of doing same things in API or else user is confused whether to use
actually I wrote this issue while working on #8243 where the context was actually |
I did a bit more digging and found that many filter implementations exploit the fact that whether they are working on values with extraction fn applied and make few optimizations . so, I think it is ok for filters to have extraction fn as fields. |
they were added before
VirtualColumn
was introduced and I think a better alternative now could be to have a extractionFn basedVirtualColumn
that could be used and expression based virtual column exists already.That way, all the filter and aggregator implementations don't need to handle them in different places.
The text was updated successfully, but these errors were encountered: