feat(event-explorer): column aliases #15861
Conversation
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
| select_input.append(f"tuple({', '.join(SELECT_STAR_FROM_EVENTS_FIELDS)})") | ||
| elif col.split("--")[0].strip() == "person": | ||
| select_expr = f"tuple({', '.join(SELECT_STAR_FROM_EVENTS_FIELDS)})" | ||
| elif col.split("--")[0].split(" as ")[0].strip() == "person": |
There was a problem hiding this comment.
This as is the main change in this file. You can now alias person as "whatever" if you want to
| # Make sure each column has an alias | ||
| if isinstance(expr, ast.Alias): | ||
| select.append(expr) | ||
| elif isinstance(expr, ast.Field): | ||
| select.append(ast.Alias(expr=expr, alias=expr.chain[-1])) | ||
| else: | ||
| select.append(ast.Alias(expr=expr, alias=col)) |
There was a problem hiding this comment.
This is the second fix in this file.
Sorting order by bla as x gives an error. Thus we must extract the expression from the alias. However there's a benefit in explicitly providing an alias for every selected column: we can use the generated AST in a subquery. I ran into it when writing the test that is/was #15859 . Making everything an alias here also simplifies the order by code below.
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
I watered this PR. Ready for a re-review.
Hmm.. Few reasons why I didn't go this route:
🤷 |
| select_input.append(f"tuple({', '.join(SELECT_STAR_FROM_EVENTS_FIELDS)})") | ||
| elif col.split("--")[0].strip() == "person": | ||
| select_expr = f"tuple({', '.join(SELECT_STAR_FROM_EVENTS_FIELDS)})" | ||
| elif col.split("--")[0].split(" as ")[0].strip() == "person": |
| person_indices: List[int] = [] | ||
| for index, col in enumerate(select_input_raw): | ||
|
|
||
| select: List[ast.Alias] = [] |
There was a problem hiding this comment.
Nit: This would be a bit clearer as select_columns
| # Convert star field from tuple to dict in each result | ||
| if "*" in select_input_raw: | ||
| star_idx = select_input_raw.index("*") | ||
| if "*" in select_input: | ||
| star_idx = select_input.index("*") |
There was a problem hiding this comment.
"asterisk" would be more consistent with the rest of the code than "star"
| if (query.match(/ as (`[^`]+`|"[^"]+"|[a-zA-Z$][a-zA-Z0-9_$]*)\s*$/)) { | ||
| const comment = query.split(' as ').pop()?.trim() || query | ||
| if ((comment.startsWith('`') || comment.startsWith('"')) && comment.endsWith(comment[0])) { | ||
| return comment.slice(1, -1) | ||
| } | ||
| return comment | ||
| } | ||
| if (query.includes('--')) { | ||
| return query.split('--').pop()?.trim() || query | ||
| } | ||
| return query | ||
| } | ||
|
|
||
| export function removeExpressionComment(query: string): string { | ||
| /** | ||
| * Removes the comment and/or alias from a query, keeping just the SQL. For display only! | ||
| * | ||
| * This is a rather naive implementation using basic string operations. | ||
| * Its output is not cleaned for usage in queries! | ||
| */ | ||
| export function removeCommentOrAlias(query: string): string { | ||
| if (query.includes('--')) { | ||
| return query.split('--').slice(0, -1).join('--').trim() | ||
| query = query.split('--').slice(0, -1).join('--').trim() | ||
| } | ||
| if (query.match(/ as (`[^`]+`|"[^"]+"|[a-zA-Z\$][a-zA-Z0-9\_\$]*)$/)) { |
There was a problem hiding this comment.
These two regexes only seem to differ by "_" and "$" being escaped in character sets, and whitespace being allowed at the end of the extraction one. Are these differences important, or should these be the same? I think this would be clearer if the regexes were put in file-level constants.
BTW I think the case insensitivity flag is missing, this wouldn't match the valid "AS" or "As"
|
Also some tests are failing |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Problem
Some column titles can get really long unless using the (undocumented) hidden comment syntax like
... -- Url / Screen. It would be great if this would also work with aliases.Changes
How did you test this code?
In the browser. WIP