search

hail-is / hail

Cloud-native genomic dataframes and batch computing
https://hail.is
MIT License
984 stars 246 forks source link

[query, bugfix] TableAggregate interpret supports globals in init op #14673

Closed patrick-schultz closed 2 months ago

patrick-schultz commented 2 months ago

There was a typo in the Interpret rule for TableAggregate which had it refer to the row instead of the globals inside the init op.

I tried to add a test for this, but it's frustratingly difficult to force the compiler to go through this code path. Even when using an InterpretOnly compilation, the lowering pipeline often lifts TableAggregate to a RelationalLet, and then evaluates it, using the compiler not the interpreter. This is a deeper issue we should address, but a user is currently blocked on this bug so I don't want to hold it up.

patrick-schultz commented 2 months ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @patrick-schultz and the rest of your teammates on Graphite Graphite

patrick-schultz commented 2 months ago

Can you lock down this behaviour with a test?

As I said in the description, I tried hard to write a test, but I couldn't manage to find a way to exercise this with a targeted test. And this bug is currently blocking a user, so I want to get it released asap. I can try again, but I suspect it would end up being a day or two of extra work, and would likely still end up brittle and unsatisfactory. I'd rather spend my effort getting back into the line of work that would eventually make it much easier to target specific compiler code paths.