[Explore vis]add data transformation panel#11944
Conversation
a5dd0e7 to
dff32e0
Compare
aa750d5 to
84b4f1c
Compare
|
Persistent review updated to latest commit 84b4f1c |
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
84b4f1c to
af57f2c
Compare
|
Persistent review updated to latest commit af57f2c |
af57f2c to
b66561a
Compare
|
Persistent review updated to latest commit b66561a |
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
b66561a to
ec60b3e
Compare
|
Persistent review updated to latest commit ec60b3e |
| const pipeline = transformationService.pipeline$.getValue(); | ||
| const serializedPipeline = pipeline.map((instance) => ({ | ||
| definitionId: instance.definition_id, | ||
| instanceId: instance.instance_id, |
There was a problem hiding this comment.
why need to persist instance_id? It seems it's purely a runtime identifier
There was a problem hiding this comment.
Yes, we don't need to save it, will delete it
| splitField: visConfig?.splitField, | ||
| splitLayout: visConfig?.splitLayout, | ||
| showSplitLabel: visConfig?.showSplitLabel, | ||
| dataTransformationJSON: JSON.stringify(serializedPipeline), |
There was a problem hiding this comment.
Curious why does it need JSON.stringify() for serializedPipeline?
There was a problem hiding this comment.
We don’t need to double-stringify this, I assume it is the leftover code from the initial changes.
| }, | ||
| [services, dispatch] | ||
| ); | ||
| }, [visualizationBuilder, results, pipeline]); |
There was a problem hiding this comment.
VisualizationContainer should not known the exists of pipeline, better if we can decouple transformation pipeline from VisualizationContainer.
I guess you added pipeline to deps array because you want it to trigger handleData to apply data transformations. But I think this should be VisualizationBuilder internal logic.
There was a problem hiding this comment.
Got it, VisualizationBuilder will do the subscription.
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
|
Persistent review updated to latest commit 1fe50fd |
Hailong-am
left a comment
There was a problem hiding this comment.
Review Comments
1. Mutation inside applyPipeline (transformation_service.ts:148)
if (cleaned !== instance.config) {
instance.config = cleaned; // mutates the object inside the BehaviorSubject
pipelineChanged = true;
}This directly mutates an object that's currently held by pipeline$. While it won't cause an infinite loop (the debounced re-invocation self-terminates because the cleaned config becomes stable), it violates the otherwise-immutable contract established by updateTransformationConfig, removeTransformation, etc. (which all create new objects).
The practical risk: any code that captured a reference to pipeline$.getValue() before applyPipeline ran would see its instances mutated in place unexpectedly.
Suggested fix — use an index-based loop and clone the instance:
let pipelineChanged = false;
const updatedInstances = [...instances];
for (let i = 0; i < updatedInstances.length; i++) {
let instance = updatedInstances[i];
schemaMap.set(instance.instance_id, currentSchema);
if (instance.validateConfig) {
const cleaned = instance.validateConfig(instance.config, currentSchema);
if (cleaned !== instance.config) {
instance = { ...instance, config: cleaned };
updatedInstances[i] = instance;
pipelineChanged = true;
}
}
// ...rest of loop
}
if (pipelineChanged) {
this.pipeline$.next(updatedInstances);
}2. Unused transformationService prop on VisualizationContainer
In visualization_editor_bottom_left_container.tsx:
return <VisualizationContainer transformationService={transformServices} />;But VisualizationContainer is defined with React.memo() and uses hooks internally — it never declares or consumes a transformationService prop. This prop is silently ignored (React doesn't error on extra props). It should be removed to avoid confusion.
| * transformation catalog management | ||
| */ | ||
| registerDefinition<TConfig>(definition: TransformationDefinition<TConfig>): void { | ||
| this.definitions.set(definition.id, definition as TransformationDefinition); |
There was a problem hiding this comment.
if id is duplicate, should we log a warning message here?
There was a problem hiding this comment.
Thanks for the suggestion,fixed
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
|
Persistent review updated to latest commit a6f9deb |
2 similar comments
|
Persistent review updated to latest commit a6f9deb |
|
Persistent review updated to latest commit a6f9deb |
Description
This pr adds data transformation panel:
Data Transformation is a new feature added to Explore visualization editor that allows users to reshape query results before they reach the visualization layer—without changing the underlying query.
Transformations are applied through a user-defined pipeline, where each step builds on the output of the previous one. This creates a flexible, composable workflow for manipulating data directly within the editor.
RFC:
#11928
Issues Resolved
Screenshot
Testing the changes
Check List
yarn test:jestyarn test:jest_integration