geo: allow arbitrary precision when encoding to json #124312
Conversation
rharding6373
requested review from
a team and
mgartner
and removed request for
a team
|
This change is |
There was a problem hiding this comment.
This is going to have some affect on the geo_builtins (at the very least need to clean up the info on the precision there), but wanted to get someone else's opinion on whether we want to treat the builtins with a different precision than other json encoded geo types.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mgartner)
rharding6373
added
backport-23.2.x
rharding6373
force-pushed
the
20240516_geo_json_prec_124175
branch
from
9347d0f
to
84ed6c4
Compare
There was a problem hiding this comment.
I have for the moment decided to remove the documentation about the default precision from the builtin info. PTAL.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/geospatial line 280 at r1 (raw file):
st_asgeojson(g)::JSONB->'coordinates', st_asgeojson(tbl.*, 'g')::JSONB->'geometry'->'coordinates', st_asgeojson(tbl.*, 'g', 4)::JSONB->'geometry'->'coordinates'
Note that the output for the last one is wrong. I filed #124368 after verifying that it was present in 24.1.0 and not introduced by this change. It's a regression from 23.2.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/geospatial line 280 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Note that the output for the last one is wrong. I filed #124368 after verifying that it was present in 24.1.0 and not introduced by this change. It's a regression from 23.2.
It looks like the second row is inconsistent with PG too, where I get:
?column? | ?column? | ?column? | ?column?
--------------------------------+--------------------------------+--------------------------------+----------------------
[-123.45, 12.3456] | [-123.45, 12.3456] | [-123.45, 12.3456] | [-123.45, 12.3456]
[-123.456789012, 12.345678901] | [-123.456789012, 12.345678901] | [-123.456789012, 12.345678901] | [-123.4568, 12.3457]
[-123.456789012, 12.345678901] | [-123.456789012, 12.345678901] | [-123.456789012, 12.345678901] | [-123.4568, 12.3457]
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:
-- commits line 14 at r1:
nit: add category (probably sql change).
pkg/sql/logictest/testdata/logic_test/geospatial line 280 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
It looks like the second row is inconsistent with PG too, where I get:
?column? | ?column? | ?column? | ?column? --------------------------------+--------------------------------+--------------------------------+---------------------- [-123.45, 12.3456] | [-123.45, 12.3456] | [-123.45, 12.3456] | [-123.45, 12.3456] [-123.456789012, 12.345678901] | [-123.456789012, 12.345678901] | [-123.456789012, 12.345678901] | [-123.4568, 12.3457] [-123.456789012, 12.345678901] | [-123.456789012, 12.345678901] | [-123.456789012, 12.345678901] | [-123.4568, 12.3457]
I think this is because stAsGeoJSONFromTuple no longer uses numDecimalDigits argument (as of 6009141).
There was a problem hiding this comment.
Hm, upon closer reading of the code, I feel like the right solution might be to only adjust tree.AsJSON codepath. For example, according to https://postgis.net/docs/ST_AsGeoJSON.html st_asgeojson builtin defaults to 9 precision when the argument is not specified. In other words, perhaps we should audit every place where DefaultGeoJSONDecimalDigits is used and decide on a case-by-case basis whether 9 or -1 is used?
Reviewable status:
There was a problem hiding this comment.
Good catch. With your change in place, we could pass 9 for max_decimal_digits instead of the default, or we could keep the dfault as 9 and make a new constant for full precision and use that for non-builtin calls to ToJSON(). WDYT?
Reviewable status:
There was a problem hiding this comment.
I think we should move geo.DefaultGeoJSONDecimalDigits back into builtins package (and unexport it), keep that constant to be 9 for builtins, and then specify -1 for two ToJSON calls in tree.AsJSON. We could define -1 as a separate exported constant in tree package and then also use it for two calls to geo.SpatialObjectToGeoJSON in eval.performCastWithoutPrecisionTruncation.
Reviewable status:
rharding6373
force-pushed
the
20240516_geo_json_prec_124175
branch
from
84ed6c4
to
69a36cf
Compare
There was a problem hiding this comment.
Done, except I put the full precision constant for -1 in the geo package, which seems like a better home for it than tree since it's geo-specific. Let me know if you have strong feelings and I'll move it. RFAL
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: add category (probably
sql change).
done
pkg/sql/logictest/testdata/logic_test/geospatial line 280 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I think this is because
stAsGeoJSONFromTupleno longer usesnumDecimalDigitsargument (as of 6009141).
I think this looks ok now.
There was a problem hiding this comment.
Placement sounds good to me - my only concern was not introducing dependency on geo into util/json (which 6009141 was fixing).
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status:
We used to set a default limit of 9 digit precision when converting geo datums to json. However, some users expect higher precision in json. This PR changes the default precision to -1, which allows arbitrary precision with our geo library. Because of truncation when the data enters the database, there is a limit to the precision, but now the json output matches the database data. Epic: none Fixes: cockroachdb#124175 Release note(sql change): Do not limit precision when encoding geo data types to JSON.
rharding6373
force-pushed
the
20240516_geo_json_prec_124175
branch
from
69a36cf
to
9a99313
Compare
|
Unit test failure is known issue. TFTRs! bors r+ |
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 9a99313 to blathers/backport-release-23.1-124312: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. error creating merge commit from 9a99313 to blathers/backport-release-23.2-124312: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.2.x failed. See errors above. error creating merge commit from 9a99313 to blathers/backport-release-24.1-124312: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 24.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
We used to set a default limit of 9 digit precision when converting geo datums to json. However, some users expect higher precision in json. This PR changes the default precision to -1, which allows arbitrary precision with our geo library. Because of truncation when the data enters the database, there is a limit to the precision, but now the json output matches the database data.
Epic: none
Fixes: #124175
Release note: Do not limit precision when encoding geo data types to JSON.