[query] Fix deserialization of IR function parameter names in QoB

[daniel-goldstein]

I believe #14547 introduced a bug that broke IR function deserialization in QoB by changing value_parameter_names from Array[String] to Array[Name]. This fixes the issue by pushing the introduction of the Name wrapper object to after deserialization. Another option is to incorporate the { str: String } structure of Name into the python -> scala payload, but I'm not sure I see a use case for that and we can always do that later (there is no issue of backwards compatibility with this communication between python and scala).

My main concern here is that clearly this isn't tested. I'd appreciate guidance on the current advised practice for testing this behavior.

[daniel-goldstein]

Example error:

Caused by: is.hail.relocated.org.json4s.MappingException: No usable value for value_parameter_names
No usable value for str
Did not find value which can be converted into java.lang.String
	at is.hail.relocated.org.json4s.reflect.package$.fail(package.scala:53)
	at is.hail.relocated.org.json4s.Extraction$ClassInstanceBuilder.org$json4s$Extraction$ClassInstanceBuilder$$buildCtorArg(Extraction.scala:638)
	at is.hail.relocated.org.json4s.Extraction$ClassInstanceBuilder$$anonfun$3.applyOrElse(Extraction.scala:689)
	at is.hail.relocated.org.json4s.Extraction$ClassInstanceBuilder$$anonfun$3.applyOrElse(Extraction.scala:688)
	at scala.PartialFunction.$anonfun$runWith$1$adapted(PartialFunction.scala:145)
	at scala.collection.mutable.ResizableArray.foreach(ResizableArray.scala:62)
	at scala.collection.mutable.ResizableArray.foreach$(ResizableArray.scala:55)
	at scala.collection.mutable.ArrayBuffer.foreach(ArrayBuffer.scala:49)
	at scala.collection.TraversableLike.collect(TraversableLike.scala:407)
	at scala.collection.TraversableLike.collect$(TraversableLike.scala:405)
	at scala.collection.AbstractTraversable.collect(Traversable.scala:108)
	at is.hail.relocated.org.json4s.Extraction$ClassInstanceBuilder.instantiate(Extraction.scala:688)
	at is.hail.relocated.org.json4s.Extraction$ClassInstanceBuilder.result(Extraction.scala:767)
	at is.hail.relocated.org.json4s.Extraction$.$anonfun$extract$10(Extraction.scala:462)
	at is.hail.relocated.org.json4s.Extraction$.$anonfun$customOrElse$1(Extraction.scala:780)
	at scala.PartialFunction.applyOrElse(PartialFunction.scala:127)
	at scala.PartialFunction.applyOrElse$(PartialFunction.scala:126)
	at scala.PartialFunction$$anon$1.applyOrElse(PartialFunction.scala:257)
	at is.hail.relocated.org.json4s.Extraction$.customOrElse(Extraction.scala:780)
	at is.hail.relocated.org.json4s.Extraction$.extract(Extraction.scala:454)
	at is.hail.relocated.org.json4s.Extraction$.org$json4s$Extraction$$extractDetectingNonTerminal(Extraction.scala:482)
	at is.hail.relocated.org.json4s.Extraction$CollectionBuilder.$anonfun$mkCollection$1(Extraction.scala:492)
	at scala.collection.immutable.List.flatMap(List.scala:366)
	at is.hail.relocated.org.json4s.Extraction$CollectionBuilder.mkCollection(Extraction.scala:490)
	at is.hail.relocated.org.json4s.Extraction$CollectionBuilder.result(Extraction.scala:527)
	at is.hail.relocated.org.json4s.Extraction$.$anonfun$extract$9(Extraction.scala:440)
	at is.hail.relocated.org.json4s.Extraction$.$anonfun$customOrElse$1(Extraction.scala:780)
	at scala.PartialFunction.applyOrElse(PartialFunction.scala:127)
	at scala.PartialFunction.applyOrElse$(PartialFunction.scala:126)
	at scala.PartialFunction$$anon$1.applyOrElse(PartialFunction.scala:257)
	at is.hail.relocated.org.json4s.Extraction$.customOrElse(Extraction.scala:780)
	at is.hail.relocated.org.json4s.Extraction$.extract(Extraction.scala:440)
	at is.hail.relocated.org.json4s.Extraction$ClassInstanceBuilder.buildMandatoryCtorArg(Extraction.scala:661)
	at is.hail.relocated.org.json4s.Extraction$ClassInstanceBuilder.org$json4s$Extraction$ClassInstanceBuilder$$buildCtorArg(Extraction.scala:634)
	... 39 more

[patrick-schultz]

Oof, thanks for catching that. I think the right fix is actually to push the conversion from string to name into pyRegisterIRForServiceBackend. I'll think more about the right way to test this as well.

[patrick-schultz]

So I think the root issue here is the unnecessary duplication between pyRegisterIR and pyRegisterIRForServiceBackend. The only real difference is that one takes and already parsed IR, and the other takes a string and calls the parser.

The callers of pyRegisterIR in python all call into the parser first, but I don't see any reason it has to make two calls across the python/scala bridge; I think pyRegisterIR should just take the IR as a string and call the parser like pyRegisterIRForServiceBackend does.

With that change, it should be possible to make one a simple wrapper around the other (or maybe even get rid of pyRegisterIRForServiceBackend completely). That way the core logic is shared between backends and is getting tested.

Let me know if you want help with this, or if you'd like me to make a separate PR for this.

[patrick-schultz]

Also, I'm not sure why test_define_function doesn't fail in the service backend tests. We should figure that out. If we could modify that to fail before this fix, I think that would be enough to merge this.

[daniel-goldstein]

Ah, test_define_function isn't marked as part of the QoB test suite. It needs a @qobtest

[patrick-schultz]

Ah, right!

believe I implemented your suggestion

[patrick-schultz]

Overall looks great, thanks for cleaning this up! Just one question about code duplication.

addressed