Adding new Presto functions in presto-native-execution module vs velox repo

[aditi-pandit]

Many Presto functions were added recently. A lot of them are math/probability functions. Another category is Window functions with different options for frames, IGNORE NULLs.

A common concern during reviews is that there isn't confidence in correctness wrt Presto behavior. Its only after folks write Presto e2e tests that issues might come up, and even with that there isn't fuzzer level testing in Prestissimo to feel GA level confidence.

So an idea came up to change our current developer workflow. The process would be:

• Land new functions in presto-native-execution module first
• Thoroughly test wrt Presto (with a fuzzer level tester also)
• Upgrade to Velox for common engine usage post the validation.

Would be great to hear others thoughts about this proposal.

@pedroerp, @mbasmanova, @majetideepak, @amitkdutta

[majetideepak]

I think this is a great proposal. On top of the benefits you mentioned, this will also reduce the review burden on the Velox team.
Velox fuzzer can be abstracted and we can add a function fuzzer test in Prestissimo CI.
Moving functions from Prestissimo to Velox can be done lazily on a case by case basis.

[mbasmanova]

Thoroughly test wrt Presto (with a fuzzer level tester also)

I'm curious to learn more about this. Is there any design doc or GitHub issue I can read?

[aditi-pandit]

@mbasmanova : The starting idea was to use the Velox fuzzer but swap the verification with Presto runs instead. Of course this can be done today also without any change of the developer workflow.

At a SQL level also there is some consideration to add fuzzer testing. I like the approach outlined https://www.cockroachlabs.com/blog/testing-random-valid-sql-in-cockroachdb/. There are other tools of which https://github.com/anse1/sqlsmith and https://www.sqlite.org/sqllogictest/doc/trunk/about.wiki seem most popular.
There are some folks looking at https://github.com/sqlancer/sqlancer as well.

The idea was to add a testing pipeline that runs on demand or at a regular cadence (once a week ?) separately from the build for this.

We are not settled on a solution. Would be great to hear your and other ideas also since you have invested a lot of time in this.

[mbasmanova]

The starting idea was to use the Velox fuzzer but swap the verification with Presto runs instead. Of course this can be done today also without any change of the developer workflow.

It would be nice to figure out how to do that. Ideally this would be a separate initially not tied to changing the dev process and be a pre-requisite to such.

[amitkdutta]

This is good idea @aditi-pandit. My thought is following. Velox is a leaf level dependency in compute stack. It's under heavy active development (both testing and features), and many improvements will touch functions core signature, data structures and so on. If we build some functions inside Prestodb based on a submodule of velox, it's possible that it will be very complex to merge it back to velox main branch.

There is the testing issue as you mentioned. Its possible that velox testing skip some corner cases today, however a lot of work is going to improve the fuzzing and other tests in velox (CC: @kagamiori @Yuhta @pedroerp). Generally, each compute layer will wrap with more tests which might unfold integration issues.

[aditi-pandit]

@amitkdutta : Some thoughts :

• The Velox fuzzer is a very good testing tool. I feel we should take an adaptation of it to the Presto level for Presto correctness testing. We have been using DuckDB which is also under active development and keeps fixing issues and testing. During window function development we found several deviations from Presto.
• I agree that Velox is a leaf level dependency in the compute stack. Since it is used in multiple engines we should try to maintain stable interfaces especially for functions (since there are tons of them across all engines). If function interfaces change then there should be some process or utility (ideally automated) to migrate existing functions to new signatures. Is there some large scale code change (LSC) tools used internally at Meta for such changes ?
• Its possible that there are code changes when migrating a Presto function to Velox. Code-review also would introduce some changes. But that would be a one time thing for each function. Yes, this is an overhead with the described approach,

Would be great to hear your feedback.

[amitkdutta]

@aditi-pandit
For a presto release, we ensure following things

• Make sure trunk health (all unit tests) are good
• Take a build and run with production queries with Presto verifier. Match results with previous release. This is the most valuable tool since it gives us the most confidence about correctness.

Now Presto native is a new stack. Along with above two items that is always done for java stack, we do another thing

• We shadow production queries in a test environment for 2/3 days. Then we match the performance metrics with production (usually wall time and CPU time), and look for crashes, hangs and unusual failures. This has been immensely helpful to give confidence on a Presto native build. This process discovered many bugs, reliability and performance issues with Presto and Velox stack that unit, presto e2e tests did not cover. As a new stack, we need to ensure the process health stays reliable for longer period of time without deadlocks, abnormal queue growth, slow memory leaks - while performing at a stable throughput.

Another thought I have to test velox end to end, is somehow collect or generate plans from various workload and run through it nightly. It can be like fuzzer, but tests concurrency, throughout and reliability for many parts of velox including task, driver, operator and also provides signal about performance.

[pedroerp]

Hi guys, thanks for bringing up this discussion. I'm trying to wrap my head around the proposal, but it seems like there are two related aspects:

1. Where we place engine-specific extensions. We discussed this in the past and the sentiment was that engine-specific extensions should live with the engines themselves, unless they are components we would like to share across engines. Considering Presto-specific functions are needed to execute Presto dialect (which we use not only within Presto), we discussed that Presto functions (and Spark, for that matter) would live within the Velox library itself.

2. Testing functions in the Velox library (leaf, as @amitkdutta phrased it above), or up the stack within the engines. My mental model is the following: by definition, Velox should provide a wider API than what is used by the engines (Presto). In other words, Presto can only exercise code paths that are exposed by Velox. Is the problem that engines are findings bugs on Velox code paths because they are not being exercised by the fuzzer? If so, could we extend the fuzzer to also cover such cases? Ideally, assuming that the Fuzzer does a great job at exercising the internal code paths within functions, what would be left for engines to test are integration points (can you execute Velox functions end to end, but not necessarily testing every combination of inputs)