H3 CLI Hierarchical Subcommands #846
Conversation
|
I think this may be why the tests are failing. It's setting up a build directory that is not the CWD, while the files are using relative paths. I need to see how to get the directory they're actually being put in vs where |
|
Still something funky with Windows. I'll need to dust off my VM. |
|
Hmm... There was another error that I didn't notice before. https://github.com/uber/h3/actions/runs/9521371842/job/26248637576?pr=846#step:7:301 Let me see if I can figure that out. |
|
So that last failure was legit, and was a copy-paste error when I set up arg parsing in one of the new subcommands. I have no idea what black magic the GNU libc is doing under the hood to get that casting to work "correctly" on Linux. |
| "Resolution, 1-15 inclusive, excluding 0 as it can " | ||
| "never be a child"}; |
There was a problem hiding this comment.
could cellToChildren be called with res 0 if the input cell is res 0? It would only return the input cell.
src/apps/filters/h3.c
Outdated
| int cellStrsOffset = 0; | ||
| int cellsOffset = 0; | ||
| int cellsLen = 128; |
tests/inputfiles/compact_test3.txt
Outdated
| @@ -0,0 +1 @@ | |||
| 85283473fffffff85283447fffffff8528347bfffffff85283463fffffff85283477fffffff8528340ffffffff8528340bfffffff85283457fffffff85283443fffffff8528344ffffffff852836b7fffffff8528346bfffffff8528346ffffffff85283467fffffff8528342bfffffff8528343bfffffff85283407fffffff85283403fffffff8528341bfffffff | |||
There was a problem hiding this comment.
Do we really want to support this kind of fixed length record format? This seems like the worst of human readability from binary files (hard to know if the records are the right size, as opposed to e.g. JSON or CSV where you can eyeball and see where the record separators are) and parsing performance of text files. (can't just directly reference this as a buffer of (u)int64, must parse to integers)
There was a problem hiding this comment.
Yes, supporting this did complicate the parsing code. If I can assume any kind of delimiter between records it simplifies things a lot.
But I was envisioning someone piping a script that spews out H3 indexes into this tool and they forgot to use println instead of print (or whatever) and this is how that would look, and we technically can parse it since we know exactly how many characters should be in the stream for each index.
There was a problem hiding this comment.
I simplified it a bit (and moved it to a singular function), but I couldn't reduce the complexity as much as I wanted to because sscanf returns the number of matched variables, not the number of matched characters in the string, so I can't advance the string in the obvious way.
I am half-tempted to rewrite it without any sscanf call and build the integer manually.
…libc doing?" This reverts commit 7549c75.
Co-authored-by: Isaac Brodsky <isaac@isaacbrodsky.com>
f2cfebd to
3880fe2
Compare
|
Just rebased to get the new tests run on this branch. Since I have two approvals if these new tests also pass I'll merge this. |
This adds all of the hierarchical subcommands. Everything was easy until I reached
compactCellsanduncompactCells.I am not happy with the way the flexible parser ended up in a soup of looping constructs, but I couldn't figure out any way that wasn't worse in some ways (eg, using
gotoand labels). I also couldn't figure out how handle a completely undelimited input file without copying the characters to a temporary string since you can have more than 15 hex characters in a 64-bit integer, so it would want to slurp up follow-on characters. We can simplify a bit if you think an undelimited deluge of hex characters is not a reasonable input format we should handle.I also didn't DRY the huge similarities between the two functions. I am willing to do that, but only after we agree on how the string parsing logic should look like.