fix: column preview types not accurate #1702
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1702 +/- ##
==========================================
+ Coverage 46.65% 46.67% +0.01%
==========================================
Files 593 606 +13
Lines 36467 36875 +408
Branches 9135 9267 +132
==========================================
+ Hits 17014 17211 +197
- Misses 19401 19612 +211
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
So there's a problem I didn't realize when assigning this ticket - we don't actually know it's a preview column, the column description is used on non-preview column types as well. For example, on rollup tables:
from deephaven import read_csv, agg
insurance = read_csv("https://media.githubusercontent.com/media/deephaven/examples/main/Insurance/csv/insurance.csv")
agg_list = [agg.avg(cols=["bmi", "expenses"])]
by_list = ["region", "age"]
test_rollup = insurance.rollup(aggs=[], by=by_list, include_constituents=True)
insurance_rollup = insurance.rollup(aggs=agg_list, by=by_list, include_constituents=True)
If you hover over expenses, you'll see there's an actual description:
So we don't want to swap the description in that case.
I think we need to push back on this ticket as, and either have a specific field detailing the preview type (rather than stuffing it in the description). Then we can treat it a little more differently (and also disable stuff like filtering, rather than trying to filter it as a string).
Right now the description is getting set in the API for column previews: https://github.com/deephaven/deephaven-core/blob/01b6cf49dad3e855067f0cb411a858c89b875d2d/engine/table/src/main/java/io/deephaven/engine/table/impl/preview/ColumnPreviewManager.java#L121
We have tickets on the Core side for this: deephaven/deephaven-core#3358
and deephaven/deephaven-core#2102
So I don't think we can complete this ticket as desired until addressing those two tickets first.
| const description = column.description === null ? null : column.description; | ||
| let columnType = column.type.substring(column.type.lastIndexOf('.') + 1); | ||
| let description; | ||
| if (column.description != null && column.description !== undefined) { |
There was a problem hiding this comment.
This check is redundant. != null will mean that !== undefined is always true. You'd only need to strictly check against undefined if you strictly checked against null, e.g. !== null instead of just != null.
When checking against both null and undefined, doing a loose check != null is fine.
|
Waiting on core tickets mentioned above to go through first |
Closes #1597