Skip to content
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

Get rid of the strange looking 0 following "Running..." and "runtime" #7099

Merged
merged 7 commits into from
Sep 19, 2024

Conversation

zachliu
Copy link
Contributor

@zachliu zachliu commented Aug 1, 2024

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

In certain situations such as querying parquet data using Athena, it is possible for the query engine to scan 0 byte of data because of the presence of metadata. Currently when this happens, there is a strange "0" showing up in the UI:

  • When query is running:
    2024-08-01_14-21_1

  • When query finishes running:
    2024-08-01_14-21

This is because the

        {queryResultData.metadata.data_scanned && (
          <span className="m-l-5">
            Data Scanned
            <strong>{prettySize(queryResultData.metadata.data_scanned)}</strong>
          </span>
        )}

block ends up being the "0".

After applying the code changes in the PR:

  • When query is running:
    2024-08-01_14-22

  • When query finishes running:
    2024-08-01_14-21_2

The queryResultData.metadata.data_scanned && is unnecessary since the prettySize is able to deal with null values

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

See attached screenshots above.

@zachliu zachliu changed the title Get rid of the strange looking 0 behing "Running..." and "runtime" Get rid of the strange looking 0 following "Running..." and "runtime" Aug 1, 2024
@@ -67,10 +68,9 @@ export default function QueryExecutionMetadata({
)}
{isQueryExecuting && <span>Running&hellip;</span>}
</span>
{queryResultData.metadata.data_scanned && (
Copy link
Member

Choose a reason for hiding this comment

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

I believe the reason for having this conditional is to prevent showing "Data scanned" for data sources that don't support it, which is actually most data sources Redash supports.

How about we change this conditional to something like:

isDefined(queryResultData.metadata.data_scanned) && 

Instead of just assuming it's a boolean? This way if it's a zero it will show and if it wasn't defined, it will be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when query is executing, you'd still see

354339802-a30be3ce-c1ab-474d-97b2-132a2b74fa99
and
354339839-bf471cc5-4d42-4039-ba23-476cf48011bc

because it is defined, and it's 0

Copy link
Member

Choose a reason for hiding this comment

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

If it's defined and it's 0, with the updated version it will show "Data Scanned 0 bytes", no? Kind of same behavior as you would see with your suggested change, but only for data sources that actually define data_scanned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is what i tried (couldn't find a isDefined so i used !isUndefined)

import {isUndefined} from "lodash";

{!isUndefined(queryResultData.metadata.data_scanned) && (
  <span className="m-l-5">
    Data Scanned <strong>{prettySize(queryResultData.metadata.data_scanned)}</strong>
  </span>
)}

the answer is yes and no, there are 2 states here

  1. when query execution is done, you do see the correct message 244 rows 8 seconds runtime Data Scanned 0 bytes
  2. when query is still executing, you'll see 244 rows Running… Data Scanned 0 bytes which is a bit confusing because the query is still running

but i do see your point, with only the !isQueryExecuting, when "Data scanned" is not supported, we are seeing 3 rows 4 seconds runtimeData Scanned ?

so i think we should do both

!isUndefined(queryResultData.metadata.data_scanned) && !isQueryExecuting

zachliu and others added 2 commits September 18, 2024 12:00
otherwise we could get "Data Scanned ?" if it's not supported
by some data sources
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

👍

@arikfr arikfr merged commit 38dc31a into getredash:master Sep 19, 2024
11 checks passed
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.

3 participants