links icon indicating copy to clipboard operation
links copied to clipboard

Record extension does not work for non-literals in query

Open SimonJF opened this issue 2 years ago • 1 comments

While looking at #1124, I noticed that record extension only works for record literals in the query evaluator. The issue is that there is no representation of record extension in the query IR; it gets translated away during the IR -> Query IR pass.

However, this is not safe in general since there's nothing guaranteeing that the subject of the extension is actually a record literal. Indeed, translation does not even normalise the argument, so there are a large class of terms that should work that don't. See, for example:

var db = database "links";
var test_table = table "test_table" with (
                   id: Int,
                   integer: Int)
                 where id readonly from db;
query { for (x <-- test_table)  [( a=x.id | (if (true) { ( b=x.integer | x ) } else { ( b=x.integer | x ) }))]}

***: Error: Links_core.QueryLang.DbEvaluationError("Error adding fields: non-record") 

Even if we did try to compile away Extend on normalised terms, we still couldn't guarantee that this is a record; for example, if the if statement referred to a field in the table.

To fix, I think we will need an explicit representation of Extend, work out the necessary normalisation rules / normal forms, and the appropriate SQL implementation.

SimonJF avatar Apr 01 '22 15:04 SimonJF

Hmm. I'm not sure whether this is a bug per se, in the sense that record extension isn't included in the formalizations of how this is supposed to work, but it seems likely that it would work if we normalized the rest-of-record and added some commutation laws (e.g. if the subject of an "if" is true or false we can just reduce it, but otherwise seems like the extension could be pushed into both branches.) Relatedly @vcgalpin has asked for record field discarding (i.e. an operation _\l : (l:A|r) -> (r) for every field name l, that does what its polymorphic type says it must do if it terminates). It would be good to add this to the theory / implementation too.

A short term (not requiring additional research) fix could be to make proper occurrences of record extension "wild", so that they are not (for now) allowed in queries. So at least we don't get mysterious run-time errors.

jamescheney avatar Apr 06 '22 15:04 jamescheney