Skip to content

Conversation

@JoelColledge
Copy link

@JoelColledge JoelColledge commented Nov 12, 2021

The TOML format has a restriction that if a table itself contains
tables, all keys with non-table values must be emitted first. This led
to the following error:

$ echo '{"a": {}, "b": 1}' | rq -T
[ERROR] [rq] Encountered: TOML serialize error
[ERROR] [rq] Caused by: values must be emitted before tables

Fix this by using tables_last to output the tables after other keys.

This affects all output formats that serialize using serde. The ordering
is unimportant in the other formats, so that is harmless. The
performance impact should be small.


Fixes #194

I encountered this problem and came up with this solution. I can understand that you may not wish to merge it due to the effect on other formats. If not, any ideas how to solve it more cleanly?

The TOML format has a restriction that if a table itself contains
tables, all keys with non-table values must be emitted first. This led
to the following error:

$ echo '{"a": {}, "b": 1}' | rq -T
[ERROR] [rq] Encountered: TOML serialize error
[ERROR] [rq] Caused by: values must be emitted before tables

Fix this by using tables_last to output the tables after other keys.

This affects all output formats that serialize using serde. The ordering
is unimportant in the other formats, so that is harmless. The
performance impact should be small.
@JoelColledge
Copy link
Author

I did a basic performance test with this and measured a 4% performance degradation relative to master. The test used release builds, converting json to json. Here is the bash hackery:

{ echo -n '{' ; for i in {0..99} ; do echo -n "\"a$i\": 0, \"b$i\": {}, " ; done ; echo '"a100": {}}' ; } > /tmp/test1
last=/tmp/test1 ; for a in {1..16} ; do next=/tmp/test1-$a ; cat $last $last > $next ; last=$next ; done
for i in {1..50} ; do for version in rq-master rq-tables_last ; do echo "$version $i" >&2 ; time { cat /tmp/test1-16 | ./$version > /dev/null ; } ; done ; done 2> out1-50

@jcaesar
Copy link
Contributor

jcaesar commented Dec 27, 2022

Hm, this has the unfortunate side effect of removing length hints, which may make CBOR and similar formats longer in some cases (e.g. {"x":0}: before after). All output formats having their items in the order TOML requires is also a bit weird. I'd give TOML working the priority here, but if there is some way this can be handled in the TOML serializer, that might be nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

macOS: TOML with Table values produces error on output

3 participants