Support Scripted aggregations and improve Field Mappings #383
Support Scripted aggregations and improve Field Mappings #383V1NAY8 wants to merge 1 commit intoelastic:mainfrom
Field Mappings #383Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
jenkins test this please |
|
Hopefully now tests should pass |
|
jenkins test this please |
V1NAY8
force-pushed
the
scripted-aggs
branch
from
c9885af to
ff49b55
Compare
|
@sethmlarson Can we please work together and get this in ? Type hinting in other files is depending on this. |
… and refactor/improve capability_matrix usage
sethmlarson
left a comment
sethmlarson
left a comment
There was a problem hiding this comment.
Thanks for this! I have some questions for you:
| } | ||
| """ | ||
| agg = {func: {"field": field}} | ||
| if field in self._script_fields: |
There was a problem hiding this comment.
Since this construction is repeated 4 times below can we create a private method (_get_field_definition(field)?) that contains this logic?
| """Holds all information on a particular field in the mapping""" | ||
|
|
||
| column: str | ||
| display_name: str |
There was a problem hiding this comment.
How is column different from display_name? We should probably document that somewhere in comments if we need to have both.
| try: | ||
| # display_name can be None | ||
| index = self._mappings_capabilities[ | ||
| (self._mappings_capabilities.display_name == display_name) |
There was a problem hiding this comment.
What circumstances can have a display_name of None? Do we need this if we remove display_name in favor of column?
| Values: pd_dtype as np.dtype | ||
| """ | ||
| pd_dtypes = self._mappings_capabilities["pd_dtype"] | ||
| pd_dtypes = self._mappings_capabilities[["display_name", "pd_dtype"]].set_index( |
There was a problem hiding this comment.
Help me understand why this change is necessary, I'm sure it is but I don't see it immediately.
|
|
||
|
|
||
| class TestSeriesArithmetics(TestData): | ||
| def test_ecommerce_datetime_comparisons(self): |
There was a problem hiding this comment.
Are we losing this test case, I don't see it covered below?
| by querying directly | ||
| """ | ||
| renames_df: pd.DataFrame = self._mappings_capabilities[ | ||
| self._mappings_capabilities.apply( |
There was a problem hiding this comment.
Do we need an .apply() call here instead of a pandas boolean filter here?
3 participants
This PR Closes #267
field_mappings.pystrictly typed.capability matrixto holddisplay_namesas a column rather than index.mapping_capabilitiesto make it elegant, understandable, and easy.seriesobjects along with tests.es_infowhere we print themapping_capabilities.@sethmlarson please review it. :)
Ask Jenkins to run this once