snleee commented on PR #8462: URL: https://github.com/apache/pinot/pull/8462#issuecomment-1087990840
I have one general comment before reviewing the code. Currently, our old and new configs look like the following: ``` Current config: "tableIndexConfig": { "aggregateMetrics": "true", ... } -------------------------------- New config: "aggregationConfigs": [ { "columnName": "destColumn", "aggregationFunction": "MIN(srcColumn)" } ] ``` The old config can be translated into the new config with the same behavior like the following: ``` e.g. Schema: dimCol1 | metricCol1 | metriccol2 old config: "tableIndexConfig": { "aggregateMetrics": "true", ... } new config: "aggregationConfigs": [ { "columnName": "metricCol1", "aggregationFunction": "SUM(metricCol1)" }, { "columnName": "metricCol2", "aggregationFunction": "SUM(metricCol2)" } ] ``` So, we can actually do the following: 1. Put deprecated annotation for the old config in the table config 2. Keep backward compatibility by having a config conversion layer from the old config to the new config format 3. From the code, we can always use the new config format. It's fine that thie above can be done in multiple steps but I think that we should not keep the 2 different configs for aggregateMetrics feature. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org