-
Notifications
You must be signed in to change notification settings - Fork 617
ui: Remove "supporting columns" from SqlTable #3591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
58388fb to
ae4abeb
Compare
Remove the concept of `supporting columns` from the SqlTable. It originally served to allow richer visualisation of the data in the table (e.g. to render arg value correctly we need to know its value, as well as its type). Supporting columns allowed for multiple sql values to be fetched when rendering a given TableColumn. A much better approach is to fetch a single complex value (in protobuf or json) and render it. To support this, TableColumn.display is introduced: TableColumn.column is still considered the "canonical" underlying value (used for aggregation, casting, etc), while, if set, `TableColumn.display` is passed to `renderCell`. This patch also converts the only existing use case for "supporting columns": a new __intrinsic_serialise_arg(arg_set_id, key) SQL function is introduced, which returns a ArgValue proto. Also clean up some complexity in query generation which is no longer needed.
ae4abeb to
400cc9a
Compare
protos/perfetto/ui/args.proto
Outdated
| @@ -0,0 +1,39 @@ | |||
| /* | |||
| * Copyright (C) 2025 The Android Open Source Project | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this in the ui folder? This is a trace processor thing, it should live in the TP folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why do we need to make this a proto? It makes everything more complicated. Is see no reason this cannot be a JSON object and everyone's life is easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: ui: the main purpose of this proto is to communicate with the UI (similar to the viz package in the stdlib) — LMK if you prefer for it to live in trace_processor or trace_processor/ui or trace_processor/viz or something else.
Re: JSON: the main reason is that JSON.parse doesn't go well with int64s, so I thought that having a proto for this would be easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Lalit. We should just use JSON.
Unless I'm very much mistaken, we can just pass a custom reviver to JSON.parse() that parses all integral values to a bigint, or perhaps only those that exceed Number. MAX_SAFE_INTEGER depending on how much inconsistency you're willing to deal with in the SQL table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking to propose exactly custom receiver in JSON.parse. Feels like the no-brainer soution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched to JSON, which works for this case (I serialise ints as strings and parse them into BigInt explicitly), but a custom reviver won't work for the general case — it passes numbers to the reviver, which are already cropped, with seemingly no way to get back to the underlying value of the int.
The only option I could find is json-bigint, which seems to be manually parsing JSON in javascript, which sounds less than ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the context.source parameter in a JSON reviver in JavaScript to access the raw string.
Example from the docs above:
const bigJSON = '{"gross_gdp": 12345678901234567890}';
const bigObj = JSON.parse(bigJSON, (key, value, context) => {
if (key === "gross_gdp") {
// Ignore the value because it has already lost precision
return BigInt(context.source);
}
return value;
});Support looks good:

stevegolton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed - suggest switch back to JSON for the args
supporting columns from SqlTable
Remove the concept of
supporting columnsfrom the SqlTable. It originally servedto allow richer visualisation of the data in the table (e.g. to render arg value
correctly we need to know its value, as well as its type). Supporting columns allowed
for multiple sql values to be fetched when rendering a given TableColumn.
A much better approach is to fetch a single complex value (in protobuf or json) and
render it. To support this, TableColumn.display is introduced: TableColumn.column is
still considered the "canonical" underlying value (used for aggregation, casting, etc),
while, if set,
TableColumn.displayis passed torenderCell.This patch also converts the only existing use case for "supporting columns": a new
__intrinsic_serialise_arg(arg_set_id, key) SQL function is introduced, which returns
a ArgValue proto.
Also clean up some complexity in query generation which is no longer needed.