[Canvas] XY. Step 2. Move xDomain to the expression function and update types.#111378
[Canvas] XY. Step 2. Move xDomain to the expression function and update types.#111378Kuznietsov wants to merge 61 commits intoelastic:presentation/feature-xyfrom
xDomain to the expression function and update types.#111378Conversation
# Conflicts: # src/plugins/vis_types/xy/public/vis_component.tsx
Kuznietsov
requested review from
crob611,
markov00,
nickofthyme and
stratoula
Kuznietsov
force-pushed
the
xy_chart_step_2
branch
from
db434f3 to
4c283a2
Compare
…xy_chart_step_2 # Conflicts: # src/plugins/vis_types/vislib/public/vislib/helpers/point_series/_add_to_siri.ts # src/plugins/vis_types/xy/public/config/get_aspects.ts # src/plugins/vis_types/xy/public/config/get_axis.ts # src/plugins/vis_types/xy/public/config/get_config.ts # src/plugins/vis_types/xy/public/expression_functions/xy_vis_fn.ts # src/plugins/vis_types/xy/public/services.ts # src/plugins/vis_types/xy/public/to_ast.ts # src/plugins/vis_types/xy/public/types/param.ts # src/plugins/vis_types/xy/public/utils/accessors.tsx # src/plugins/vis_types/xy/public/vis_component.tsx # src/plugins/visualizations/common/expression_functions/xy_dimension.ts
stratoula
left a comment
stratoula
left a comment
There was a problem hiding this comment.
I haven't finished testing it, but here are some initial comments :)
| }; | ||
| } | ||
|
|
||
| const getNextToAccessorColumn = ( |
There was a problem hiding this comment.
Can you move this helper with the other accessor helpers?
There was a problem hiding this comment.
Definitely) I've forgotten this method at the build_hierarchical_data.ts file)
| return compact( | ||
| dimensions.map((d) => { | ||
| const column = d && columns[d.accessor]; | ||
| const column = d && getColumnByAccessor(columns, d.accessor); |
There was a problem hiding this comment.
This function is used a lot both in xy and vislib plugin. Can we right some tests (unit) for them?
There was a problem hiding this comment.
Nick asked for exposing those functions from elastic-charts, so, I didn't want to do that )
elastic/elastic-charts#1379
But I'll write, np.
There was a problem hiding this comment.
Added tests for most important utils at accessors.ts.
| column: { | ||
| types: ['vis_dimension'], | ||
| help: i18n.translate('visTypeXy.function.column.intervalValue.help', { | ||
| defaultMessage: 'column', |
There was a problem hiding this comment.
Can we describe a little bit better what column is exactly? I am also confused why the i18n.translate('visTypeXy.function.column.intervalValue.help' has the intervalValue key?
There was a problem hiding this comment.
@stratoula, I've added the description to all the fields.
| considerInterval: { | ||
| types: ['boolean'], | ||
| help: i18n.translate('visTypeXy.function.xDomain.considerInterval.help', { | ||
| defaultMessage: 'considerInterval', |
There was a problem hiding this comment.
Also here, can we be more specific? What does the considerInterval flag?
| min: { | ||
| types: ['number'], | ||
| help: i18n.translate('visTypeXy.function.xDomain.min.help', { | ||
| defaultMessage: 'min', |
| max: { | ||
| types: ['number'], | ||
| help: i18n.translate('visTypeXy.function.xDomain.max.help', { | ||
| defaultMessage: 'max', |
| logBase: { | ||
| types: ['string'], | ||
| help: i18n.translate('visTypeXy.function.xDomain.logBase.help', { | ||
| defaultMessage: 'logBase', |
| coordinates: { | ||
| types: ['string', 'number'], | ||
| help: i18n.translate('visTypeXy.function.xDomain.coordinates.help', { | ||
| defaultMessage: 'coordinates', |
| xDomain: { | ||
| types: ['x_domain'], | ||
| help: i18n.translate('visTypeXy.function.args.xDomain.help', { | ||
| defaultMessage: 'x_domains', |
There was a problem hiding this comment.
What are the x_domains? Let's explain it a bit!
| } | ||
| }; | ||
|
|
||
| export const getAdjustedInterval = ( |
There was a problem hiding this comment.
Q: Why are we defining this function on the common folder? As far as I can see it is used only by a public file.
Moreover, the same function exists on the charts plugin. Why don't we use this one, rather that repeating the same code?
There was a problem hiding this comment.
In the further PRs we are moving all functions to common folder, as Peter asked to do because we need to register them on server and client. Also, getAdjustedInterval is a function, which is related to our expression function x_domain and doesn't have any logical connections to chart plugin, as I can see. So, I've decided to copy it and use it in a particular expression function. And getAdjustedInterval is exported from the public folder of charts.
There was a problem hiding this comment.
Apologies for the late review. I didn’t check everything in detail (I hope to get to it next week), but at a high level:
- the changes in vislib don’t seem necessary, it looks like they move some types around and introduce helpers, but don’t change functionality. As this code will be removed in the hopefully not too distant future, I want to change it as little as possible till then - there’s very little to gain here and the chance for introducing bugs is always present
- I agree with Stratoula about the need of adding descriptions to the concepts of newly introduced expression functions. It will also make the PR easier to read
See the comment in the code - I think there’s a major issue in the setup of this expression function (how can x_domain access the datatable)edit: scratch this part, I confused myself- This PR is not described well enough in relation to the complexity of the changes it does. It should describe every major change that happened and why it had to happen
- After going through this PR I believe we should make a better plan of the intended outcome before starting large scale changes like this. I understand not everything can be planned in advance and there will be iterations but we should at least agree on a list of expression functions, their rough arguments (doesn’t need to be a perfect list), how information is passed through them and what’s the responsibility of each part (functions and the renderer). It’s not clear to me how these changes fit into the larger picture and there’s a large potential to break things. Once we have this high level target architecture it will become much easier to work through the code shaping it according to this vision. I’m happy to participate in this.
| column !== undefined | ||
| ) { | ||
| const adjusted = getAdjustedDomain( | ||
| context?.rows ?? [], |
There was a problem hiding this comment.
getAdjustedDomain needs the actual data table to return the right result. Previously this was simple because it would be part of the renderer, but as you moved it into an expression function, you need to get access to the data table in a different way. In the declaration of x_domain you simply made the datatable input optional, but in practice (in the to_ast file) it will never receive an input so it can’t do its calculations based on the data table - this is the line where you fall back to an empty array instead of the actual “rows” which always will happen. We need to find a solution for this general problem (which will only get worse as in the Lens use case a lot of things are decided based on the actual data to render instead of static arguments). I would like to do this in a proper design document rather than a large, hard to review PR
There was a problem hiding this comment.
About this issue, possibly, I'm wrong, but the function, which is going after some datasource expression function (as es_aggs, es_docs, etc.) is accepting the actual data, which came from it and spreading to all arguments, which are expressions and spreading came data to the next expression in the pipe of the expressions and it is going forward without change. If I'm wrong, I'd like to be fixed by Peter )
There was a problem hiding this comment.
And fetching new data is executing a new cycle of executing datasource expression, passing the data to the other expressions in the pipe, and updating the renderer, so, we'll have the updated data all the time ( at least, at vis_editors and canvas, I don't really know, if Lens is using it in some specific way).
There was a problem hiding this comment.
Ah, you are right, of course, thanks for explaining - shouldn’t have reviewed so late in the evening :)
There was a problem hiding this comment.
what is the reason for moving this from renderer into the expression function ? I would prefer to keep rendering logic in the actual renderer and keep the expression function simple (just providing configuration to the renderer)
There was a problem hiding this comment.
yea, I would like to do that too, but adjustedDomain is a specific logic, related to the aggregations, to be supported for vis_types. I wished to keep the renderer clean from any logic, related to the aggregations.
About this point, I've moved types to |
Definitely, I'll do that. I have forgotten about it while writing this PR and planning the other steps). |
I agree with you and I'll update the description today. |
Makes sense, thanks |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@stratoula, I've added the description for all the columns and added tests. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: cc @Kunzetsov |
8 participants
Completes part of #110430 and #101377
Step 2. XY. Move
xDomainto theexpression functionand update types.Further steps have been performed at this PR:
x_domainexpression function has been added. It has given the possibility to configure chart axis (domain) in the way the user wants without direct connection to the elastic aggregation configuration. Added support not only ofmin,maxandminIntervalfields, but alsocoordinatesandlogBaseoptions, which are supported byelastic-chartsSettingscomponent.vislibto break the link between these two visualizations and to be able to update the code atxyas is necessary.accessorof typeDatatableColumn.masterversion of the code and merged ([Viz] legend duplicates percentile options when chart has both left & right Y axes #113073).