Skip to content

Conversation

@mattnibs
Copy link
Contributor

@mattnibs mattnibs commented May 21, 2024

This commit uses the Zed service query/describe endpoint to describe to contents of a given Zed query- replacing the use of the compile endpoint and the Zed-ast library for this purpose.

Fixes #3079

@mattnibs mattnibs force-pushed the use-describe-endpoint branch 3 times, most recently from 4dc17e0 to 9713b4b Compare May 21, 2024 22:35
@mattnibs mattnibs marked this pull request as draft May 21, 2024 22:38
@mattnibs mattnibs force-pushed the use-describe-endpoint branch 6 times, most recently from a659f08 to b793fe7 Compare May 22, 2024 18:53
@mattnibs mattnibs marked this pull request as ready for review May 22, 2024 19:04
@mattnibs mattnibs force-pushed the use-describe-endpoint branch from b793fe7 to fe70154 Compare May 23, 2024 19:42
@philrz

This comment was marked as resolved.

@philrz
Copy link
Contributor

philrz commented May 30, 2024

This is effectively a note-to-self but I'll record it here in case anyone else has the same question upon reviewing these changes (cc: @jameskerr). In the ztest in brimdata/super#5126 I noticed that the head in the API call to /describe was being populated with the pool source info, but sniffing my API traffic in the Zui branch from this PR showed the from pool info going into query while head was null. I checked with @mattnibs and he confirmed that at this point the head is something primarily used to make testing easier on the Zed side and so while Zui would have the option to populate it, it's also fine/expected that for now Zui is not populating head.

@mattnibs mattnibs force-pushed the use-describe-endpoint branch 2 times, most recently from 9b7479f to abc397c Compare May 30, 2024 20:25
@mattnibs mattnibs force-pushed the use-describe-endpoint branch 2 times, most recently from 773b41a to ae6b433 Compare June 3, 2024 18:16
@mattnibs mattnibs requested a review from nwt June 3, 2024 18:16
This commit uses the Zed service query/describe endpoint to describe to
contents of a given Zed query- replacing the use of the compile endpoint
and the Zed-ast library for this purpose.
@mattnibs mattnibs force-pushed the use-describe-endpoint branch from ae6b433 to 3b4ce88 Compare June 3, 2024 18:17
Copy link
Member

@nwt nwt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good!

const {poolName, error} = info
const pool = this.select(Pools.getByName(lakeId, poolName))

if (info.error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix this.

}

if (!error && history.action === "PUSH") {
if (history.action === "PUSH") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix this too

async compile(
async describeQuery(
query: string,
pool?: string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing pool name causes leak of opaque error into stacked bar area (change at Zed 68f35c9)

5 participants