Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output nested tables last #221

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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