Change from DataSize to long to remove unnecessary data creation

[shangm2]

Description

1. This is the first step of a series of efforts to make the communication between coordinator and worker slimmer by using simple type instead of DataSize / Duration / DateTime.
2. Facebook internal test will fail due to this change but the corresponding fix is in facebook repo.

Motivation and Context

1. From profiling, we see the creation and garbage collection of certain objects can be expensive

4. These objects are designed to represent DataSize in a human readable way. However they are expensive to create and cause unnecessary allocation.

Impact

Test Plan

1. passed verifier test: https://www.internalfb.com/intern/presto/verifier/results/?test_id=212504
2. passed cpp worker verifier: https://our.internmc.facebook.com/intern/presto/verifier/results/?test_id=212506
3. make sure UI does not break

Contributor checklist

• Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
• PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
• Documented new properties (with its default value), SQL syntax, functions, or other functionality.
• If release notes are required, they follow the release notes guidelines.
• Adequate tests were added if applicable.
• CI passed.

== RELEASE NOTES ==

General Changes
* Improve scheduling by using long instead of DataSize for critical path.

[steveburnett]

Thanks for the release note! Nit suggestion rephrasing to consider, to follow the Order of changes in the Release Notes Guidelines:

== RELEASE NOTES ==

General Changes
* Improve scheduling by using long instead of DataSize for critical path.

[ZacBlanco]

I have a concern that this will break compatibility with some of the existing tooling around Presto. A good number of JSON blobs get exposed to downstream tools through the query event listener framework. Is there any plan to maintain backwards compatibility or help migrate tools that will break due to this change?

[shangm2]

I have a concern that this will break compatibility with some of the existing tooling around Presto. A good number of JSON blobs get exposed to downstream tools through the query event listener framework. Is there any plan to maintain backwards compatibility or help migrate tools that will break due to this change?

Hey @ZacBlanco we certainly dont want to break anything. Could you point me to those downstream tools so I can talk to the team and come up with a plan? Thanks a lot.

[ZacBlanco]

At first I was concerned about the event listener, however it seems to have a separate API for the objects that are returned. So that should be good. We do have a few closed-source tools which are used internally to grab information from the coordinator REST API. I think as long as we don't affect the output format of the JSONs at /v1/query/{queryId} and EXPLAIN ANALYZE queries it should be fine as that's what most of our tools use. I see that in BasicQueryStats you've already made sure that the fields serialize to DataSize when converted to JSON. After taking a closer look I think this PR is ok in the current state

[ZacBlanco]

Sorry I have been preoccupied with some other things. I will try to review again later today or tomorrow

[facebook-github-bot]

@shangm2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[yhwang]

LGTM for the UI part

[facebook-github-bot]

@shangm2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[ZacBlanco]

Thanks again for all the updates @shangm2 . If we do end up changing public endpoints (see my comment), I would request that we also make sure to update the existing openapi spec under the presto-openapi module.

I think this change should be a topic to bring up at the upcoming TSC to see how folks feel about it and whether they are OK breaking the REST API compatibility. I will defer to the community to make the final decision, but I would err on the side that we should not break the API. If you can present some compelling performance results at the TSC meeting, it might also help convince the community that this change is worth breaking API compatibility.

How does that sound to you?

[ZacBlanco]

I wonder if it would be worth adding a method in presto-common or similar called checkDataSize with the following implementation:

public long checkDataSize(long size, String name) {
    checkArgument(size >= 0, "%s is negative", name);
    return size;
}

[NikhilCollooru]

Merging this, since we got a go ahead regarding the UI changes.

Shang made the suggested changes and removed changes from spi .

[amitkdutta]

Looks good.

[NikhilCollooru]

sorry ! I missed @ZacBlanco latest comment. We will address the comments in the next PR

[NikhilCollooru]

I think this change should be a topic to bring up at the upcoming TSC to see how folks feel about it and whether they are OK breaking the REST API compatibility. I will defer to the community to make the final decision, but I would err on the side that we should not break the API. If you can present some compelling performance results at the TSC meeting, it might also help convince the community that this change is worth breaking API compatibility.

We agree, we will make sure to bring this up in the next TSC. Thanks for the review. We will also discuss internally regarding this change.