JackieTien97 commented on code in PR #816:
URL: https://github.com/apache/tsfile/pull/816#discussion_r3251950790


##########
python/tsfile/dataset/reader.py:
##########
@@ -256,28 +371,29 @@ def _metadata_tag_values(group, tag_count: int) -> tuple:
         return tuple(values)
 
     @staticmethod
-    def _metadata_field_stats(group) -> Dict[str, dict]:
-        stats = {}
+    def _metadata_field_stats(group) -> Dict[str, SeriesStats]:
+        """Collect per-measurement stats for cells that have real values.
+
+        A measurement appears in the result iff its native ``statistic`` block
+        is populated and reports a positive ``row_count``. Columns that the
+        device never wrote (Tablet skip / all-NaN pandas column) carry no
+        real values and are intentionally absent -- the dataset layer
+        surfaces only series that physically exist.
+        """
+        stats: Dict[str, SeriesStats] = {}
         for timeseries in group.timeseries:
             statistic = timeseries.statistic
-            timeline_statistic = timeseries.timeline_statistic
-            if (
-                not timeline_statistic.has_statistic
-                or timeline_statistic.row_count <= 0
-            ):
+            if not statistic.has_statistic or statistic.row_count <= 0:

Review Comment:
   The filter condition changed from checking 
`timeline_statistic.has_statistic` to checking `statistic.has_statistic`. This 
is a semantic shift: the old code filtered based on the timeline (timestamp 
axis), while the new code filters on the value statistic.
   
   Are there cases where `statistic.has_statistic` is `False` but 
`timeline_statistic.has_statistic` is `True`, or vice versa? If the two can 
diverge, this changes which series are visible. The docstring says "iff its 
native `statistic` block is populated" but the old behavior was gating on 
`timeline_statistic`.
   
   Also, line 388 now reads `timeline_statistic` unconditionally after the 
`statistic` check passes. If a measurement has `statistic.has_statistic=True` 
but `timeline_statistic.has_statistic=False` (or `timeline_statistic` is 
`None`), lines 393-395 would raise. Worth a defensive check or a note 
explaining why this can't happen.



-- 
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]

Reply via email to