modelop / hadrian

Implementations of the Portable Format for Analytics (PFA)
Apache License 2.0
130 stars 49 forks source link

Hadrian fails when numbers are used as Map keys. #42

Open jthompson6 opened 7 years ago

jthompson6 commented 7 years ago

For instance, the YAML

output: 
  type: map
  values: double
method: map
action:
  - {"new": {"0": input}, "type":{"values":"double","type": "map"}}

fails with the message "0" is not a valid symbol name. The same YAML with "a" as the map key works.

In Titus, both PFAEngine.fromJson('''{"input":"double","output":{"values":"double","type":"map"},"method":"map","action":{"new":{"0":"input"}, "type":{"values":"double","type":"map"}}}''') and PFAEngine.fromJson('''{"input":"double","output":{"values":"double","type":"map"},"method":"map","action":{"new":{"a":"input"}, "type":{"values":"double","type":"map"}}}''') work.

jpivarski commented 7 years ago

https://github.com/opendatagroup/hadrian/blob/master/hadrian/src/main/scala/com/opendatagroup/hadrian/reader.scala#L989

This readExpressionMap should have keysMustBeSymbols = false. See the definition here:

https://github.com/opendatagroup/hadrian/blob/master/hadrian/src/main/scala/com/opendatagroup/hadrian/reader.scala#L667

Not checking for valid symbols would be okay because if they're not actual field names, it will fail later in compilation.

Titus is missing this feature altogether:

https://github.com/opendatagroup/hadrian/blob/master/titus/titus/reader.py#L507

I don't know if that's a different problem.