Skip to content

Conversation

mabaasit
Copy link
Collaborator

@mabaasit mabaasit commented May 17, 2022

feat(explain-aggregation): explain aggregation COMPASS-5788

Explain:

Explain.mov

Datalake Explain:

Screen.Recording.2022-05-17.at.12.54.34.mov

Error state:

image

Description

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@mabaasit mabaasit requested a review from gribnoysup May 17, 2022 16:09
)}
<Card className={cardStyles} data-testid="pipeline-explain-results-json">
<Document
doc={doc}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we recently discovered re-creating HadronDocument in render is really not great so instead of passing a document instance created on every render, you can pass the object that the document is based on instead

Suggested change
doc={doc}
doc={plan}

The memoization of the HadronDocument instance will be handled by the Document component itself

Copy link
Collaborator

@gribnoysup gribnoysup left a comment

Choose a reason for hiding this comment

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

Seems like showExplainButton is only gated by the feature flag. Is the plan to support it in the Cloud right away?

@gribnoysup
Copy link
Collaborator

gribnoysup commented May 18, 2022

Should we disable "Explain" button in the tab header when last stage is merge or out similar to what we do for "Export" or handle it somehow? I'm not sure if explaining out/merge will actually create a collection, if it does, we probably want it just disabled?

image


const cardStyles = css({
// 170px works with minimum-height of compass
// todo: handle height for bigger sized compass
Copy link
Collaborator

@gribnoysup gribnoysup May 19, 2022

Choose a reason for hiding this comment

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

As an addition to this TODO, currently because "copy to clipboard" button is part of the Document component and not the card, the whole thing scrolls including the button:

image

I think we might want to set the height here on the document fields container and not the wrapper card as a way to prevent it, but there is no way to do it now, we would need to change the component interface a bit, and it's a very small thing so totally can be addressed later

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure :)

…explain-query-performance.tsx

Co-authored-by: Sergey Petushkov <petushkov.sergey@gmail.com>
@mabaasit mabaasit merged commit 2318df5 into main May 19, 2022
@mabaasit mabaasit deleted the COMPASS-5788-show-explain-results branch May 19, 2022 15:52
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.

4 participants