Closed edugfilho closed 2 weeks ago
sql.diff
Good to know that the other ones aren't too hard to convert. Looks like the sql tests are failing at least partially due to the ones I wrote. Have you tested these on the real data to see if the output looks ok and there are improvements? Getting them wrong and needing a backfill could be expensive and the unit tests aren't the most comprehensive
I haven't done extensive test. I'll test it, fix the code and add more tests.
sql.diff
⚠️ Only part of the diff is displayed.
sql.diff
⚠️ Only part of the diff is displayed.
sql.diff
⚠️ Only part of the diff is displayed.
sql.diff
sql.diff
⚠️ Only part of the diff is displayed.
The tests have been fixed and I tested the code with a number of instances from real data by using expressions such as assert.equal(<original_udf_in_js>(input), <new_udf_in_sql>(input))
sql.diff
sql.diff
Fixes https://github.com/mozilla/glam/issues/2840 @BenWu's suggestion in the issue above was to set up small tests where we'd replace the JS UDFs with something that does very little and see at best the potential amount of performance to gain.
Despite agreeing with the approach I thought it'd be a good idea to make the conversions at once, especially for
functional_buckets
, which will probably be more used once Glean stops sending 0-count buckets. And since there are some tests in the UDFs, I decided to take Ben's samples and converted the other GLAM UDFs. In fact there's only one UDF that hasn't been converted to SQL, because it's not trivial (here in case someone wants to take a shot at it)Checklist for reviewer:
<username>:<branch>
of the fork as parameter. The parameter will also show up in the logs of themanual-trigger-required-for-fork
CI task together with more detailed instructions.For modifications to schemas in restricted namespaces (see
CODEOWNERS
):┆Issue is synchronized with this Jira Task