[cleanup] Remove unnecessary backward-compat check in DataTable serialization by xiangfu0 · Pull Request #18002 · apache/pinot

xiangfu0 · 2026-03-27T09:47:11Z

Summary

Remove conditional backward-compat guard on CPU time / memory measurement serialization in DataTableImplV4
The inline comment explicitly noted: "The check is not needed. We can remove it. But keeping it around for backward compatibility."
All current server versions support thread resource usage reporting
Updated DataTableSerDeTest to reflect the new always-populated behavior

Test plan

./mvnw spotless:apply -pl pinot-common passes
./mvnw checkstyle:check -pl pinot-common passes
./mvnw license:check -pl pinot-common passes

🤖 Generated with Claude Code

The conditional check on CPU time and memory measurement during DataTable serialization was kept solely for backward compatibility per the inline comment. All current server versions support thread resource usage reporting, making this guard unnecessary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Keep strong assertions (> 0) for the enabled-measurement case. Use assertNotNull for the disabled case where values are always populated but may be zero. Improve comment accuracy. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

codecov-commenter · 2026-03-27T11:08:50Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.30%. Comparing base (ba0c0e0) to head (3a3b342).
⚠️ Report is 6 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #18002      +/-   ##
============================================
- Coverage     63.36%   63.30%   -0.07%     
  Complexity     1543     1543              
============================================
  Files          3200     3200              
  Lines        194114   194074      -40     
  Branches      29893    29883      -10     
============================================
- Hits         122993   122849     -144     
- Misses        61461    61587     +126     
+ Partials       9660     9638      -22

Flag	Coverage Δ
custom-integration1	`100.00% <ø> (ø)`
integration	`100.00% <ø> (ø)`
integration1	`100.00% <ø> (ø)`
integration2	`0.00% <ø> (ø)`
java-11	`63.28% <ø> (+<0.01%)`	⬆️
java-21	`63.25% <ø> (-0.06%)`	⬇️
temurin	`63.30% <ø> (-0.07%)`	⬇️
unittests	`63.29% <ø> (-0.07%)`	⬇️
unittests1	`55.53% <ø> (+<0.01%)`	⬆️
unittests2	`34.21% <ø> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

Removes the conditional “backward-compat” guard so DataTable V4 serialization always includes response serialization CPU/memory metadata, and updates tests to align with always-populated metadata behavior.

Changes:

Always write RESPONSE_SER_CPU_TIME_NS and RESPONSE_SER_MEM_ALLOCATED_BYTES metadata in DataTableImplV4#toBytes()
Update DataTableSerDeTest expectations for response-serialization metadata presence when measurement is disabled
Adjust inline test comments to reflect the new behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java	Updates assertions/comments to reflect metadata that is now always populated.
pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java	Removes conditional checks and always serializes response serialization CPU/memory metadata.

Copilot · 2026-03-27T19:18:09Z

pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java

+    // When measurement is enabled, response serialization metadata should have positive values.
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_CPU_TIME_NS.getName()));
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.SYSTEM_ACTIVITIES_CPU_TIME_NS.getName()));
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_MEM_ALLOCATED_BYTES.getName()));


The updated comment says response serialization metadata should have positive values when measurement is enabled, but the test does not assert anything about RESPONSE_SER_CPU_TIME_NS / RESPONSE_SER_MEM_ALLOCATED_BYTES in this enabled branch. Add assertions that these keys are present and validate the values (e.g., parse as long and assert > 0) or adjust the comment to match what the test actually verifies.

Copilot · 2026-03-27T19:18:09Z

pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java

+    // Response serialization metadata is always populated, but may be zero when measurement is disabled.
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_CPU_TIME_NS.getName()));
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.SYSTEM_ACTIVITIES_CPU_TIME_NS.getName()));
-    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName()));
+    Assert.assertNotNull(newDataTable.getMetadata().get(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName()));
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_MEM_ALLOCATED_BYTES.getName()));
-    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.RESPONSE_SER_MEM_ALLOCATED_BYTES.getName()));
+    Assert.assertNotNull(newDataTable.getMetadata().get(MetadataKey.RESPONSE_SER_MEM_ALLOCATED_BYTES.getName()));


Since the intent is that these fields are always populated but might be zero when measurement is disabled, asserting only NotNull is relatively weak. Consider also validating that the values are numeric (parseable as long) and match the expected disabled semantics (e.g., == 0 if guaranteed, or >= 0 if not). This makes the test more robust and ensures the serialization contract is enforced.

Copilot · 2026-03-27T19:18:10Z

pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java

+    // Add table serialization time and memory metadata.
+    getMetadata().put(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName(),
+        String.valueOf(resourceSnapshot.getCpuTimeNs()));
+    getMetadata().put(MetadataKey.RESPONSE_SER_MEM_ALLOCATED_BYTES.getName(),
+        String.valueOf(resourceSnapshot.getAllocatedBytes()));


Now that response serialization metadata is always emitted, the comment should document the expected values when measurement is disabled/unavailable (e.g., zero-filled). Adding that note here (or on the corresponding MetadataKey docs) helps downstream consumers understand whether to treat 0 differently from missing values, especially since this PR changes the old absence-based behavior.

Jackie-Jiang · 2026-03-27T19:30:09Z

pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java

-      getMetadata().put(MetadataKey.RESPONSE_SER_MEM_ALLOCATED_BYTES.getName(),
-          String.valueOf(resourceSnapshot.getAllocatedBytes()));
-    }
+    // Add table serialization time and memory metadata.


I feel we should keep the current logic and revise the comment. When CPU time/memory usage not collectable, we shouldn't return their value

Per reviewer feedback, the conditional check on CPU time and memory measurement should be kept — when these are not collectable, we should not return their values. Reverted the production code change and updated the comment to accurately describe the intended behavior instead of the old misleading TODO. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Jackie-Jiang

Please update the PR title and description

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

pinot-core/src/test/java/org/apache/pinot/core/common/datatable/DataTableSerDeTest.java:287

The metadata keys RESPONSE_SER_CPU_TIME_NS / RESPONSE_SER_MEM_ALLOCATED_BYTES are defined as LONGs, but the assertions below use Integer.parseInt, which can overflow and make this test flaky on slower/large allocations. Prefer parsing as long (Long.parseLong) and comparing using long values.

    // When ThreadCpuTimeMeasurement is enabled, responseSerializationCpuTimeNs should be positive.
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_CPU_TIME_NS.getName()));
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.SYSTEM_ACTIVITIES_CPU_TIME_NS.getName()));
    Assert.assertNull(newDataTable.getMetadata().get(MetadataKey.THREAD_MEM_ALLOCATED_BYTES.getName()));
    Assert.assertTrue(

Copilot · 2026-03-27T21:40:47Z

pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java

+    // Add table serialization time and memory metadata when the corresponding measurement is enabled.
+    // When CPU time/memory usage is not collectable, we omit these values from the metadata.
    if (ThreadResourceUsageProvider.isThreadCpuTimeMeasurementEnabled()) {
      getMetadata().put(MetadataKey.RESPONSE_SER_CPU_TIME_NS.getName(),
          String.valueOf(resourceSnapshot.getCpuTimeNs()));


This change only updates the comment, but the code still conditionally writes response serialization CPU/memory metadata based on ThreadResourceUsageProvider.isThread*MeasurementEnabled(). The PR title/description says the backward-compat guard was removed and the values are now always populated—either the implementation needs to be updated to match that behavior or the PR description/title should be corrected.

Pinot Cleanup Agent and others added 2 commits March 27, 2026 02:43

xiangfu0 added the cleanup Code cleanup or removal of dead code label Mar 27, 2026

xiangfu0 changed the title ~~Remove unnecessary backward-compat check in DataTable serialization~~ [cleanup] Remove unnecessary backward-compat check in DataTable serialization Mar 27, 2026

xiangfu0 requested review from Jackie-Jiang and Copilot March 27, 2026 19:01

Copilot started reviewing on behalf of xiangfu0 March 27, 2026 19:14 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

Jackie-Jiang reviewed Mar 27, 2026

View reviewed changes

xiangfu0 requested review from Jackie-Jiang and Copilot March 27, 2026 21:19

Jackie-Jiang approved these changes Mar 27, 2026

View reviewed changes

Copilot started reviewing on behalf of xiangfu0 March 27, 2026 21:34 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

xiangfu0 merged commit 72755a4 into apache:master Mar 27, 2026
20 checks passed

xiangfu0 deleted the cleanup-datatable-v4-compat branch March 27, 2026 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cleanup] Remove unnecessary backward-compat check in DataTable serialization#18002

[cleanup] Remove unnecessary backward-compat check in DataTable serialization#18002
xiangfu0 merged 3 commits intoapache:masterfrom
xiangfu0:cleanup-datatable-v4-compat

xiangfu0 commented Mar 27, 2026

Uh oh!

codecov-commenter commented Mar 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Jackie-Jiang Mar 27, 2026

Uh oh!

Jackie-Jiang left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

xiangfu0 commented Mar 27, 2026

Summary

Test plan

Uh oh!

codecov-commenter commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Mar 27, 2026 •

edited

Loading