obelix74 commented on PR #4115: URL: https://github.com/apache/polaris/pull/4115#issuecomment-4592922487
@flyingImer Thanks for the thorough review — addressed everything in the latest commit: * projectedFieldIds / projectedFieldNames as JSON arrays (r3313540500, r3313540506): Both fields are now List<Integer> / List<String> in ScanMetricsReport and exposed as proper JSON arrays in the OpenAPI spec. The comma-joining was leaking the JDBC storage encoding into the API shape. Since the SPI record already carried these as lists, the fix was straightforward — no secondary encoding in JSON. * totalDurationMs 0L ambiguity (r3313540509): I've added an explicit TODO on the read path tying this to the schema consolidation in #4397, where making the column nullable is the right fix. Didn't want to silently paper over the fidelity loss. * Transitional listScanMetrics / listCommitMetrics layering (r3313540512, r3318692418): Added a TODO on both methods in PolarisMetricsManager explicitly calling out that they should be rerouted once #4397 moves MetricsPersistence to its own SPI, and that the per-type split may collapse into a typed query API at that point. I agree this PR shouldn't block on #4397 — the sequencing is now documented in the code rather than just the PR thread. * API shape after merge (r3318727780): Agreed — the beta label gives us room to evolve the shape, but since the array fix was easy and clearly correct I took it here rather than deferring. @dimas-b FYI. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
