You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "willholley (via GitHub)" <gi...@apache.org> on 2023/11/02 09:36:56 UTC
[PR] mango: fix $beginsWith range [couchdb]
willholley opened a new pull request, #4828:
URL: https://github.com/apache/couchdb/pull/4828
## Overview
In the initial implementation of $beginsWith, the range calculation for view indexes mistakenly appends an integer with the size of 8 bits which gets maxed out at FF, rather than building a binary with an extra 3 bytes at the end.
Additionally, ICU defines the maximum sortable code point as `U+FFFF`. This is a more correct suffix when calculating the key range and is supported by older ICU versions (required for e.g. CentOS 7).
This commit fixes the range calculation by correctly appending the `U+FFFF` code point in the range calculation. Additionally, we use the Erlang `utf8` binary type to verify that the result is a valid utf8 string.
## Testing recommendations
`make mango-test`
## Related Issues or Pull Requests
Initial implementation: #4810
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] Documentation changes were made in the `src/docs` folder
- [ ] Documentation changes were backported (separated PR) to affected branches
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380717931
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
It turns out the illigal/replacement character `U+FFFD` actually sorts higher than `U+10FFFF`. So it would be higher than `U+10FFFF` and lower than `U+FFFF`.
```erlang
mango_json:cmp(<<16#FFFD/utf8>>, <<16#10FFFF/utf8>>).
1
```
There was a tiny chance users could have ended up with those in their data due to conversions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380608148
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
For the sake of diligence, I have launched another the full CI run with `U+FFFF` in #4829.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380652572
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
> "Erlang itself supports U+10FFFF and it is numerically greater than U+FFFF" -- while I accept that U+FFFF might be the Unicode-official marker, Erlang's support for a higher range worries me a bit.
Perhaps confusingly unicode collation rules don't sort by code point values. Higher code-points may sort lower. Besides just `U+10FFFF` and `U+FFFF` there are other such example:
```erlang
mango_json:cmp(<<16#1D175/utf8>>, <<16#41/utf8>>).
-1
```
Now, there may not be any other characters sorting higher than U+10FFFF and lower than U+FFFF, so either one is probably ok in practice, but it's a bit neater if we can use the canonical max sorting character. Jiffy / erlang / couch_util:validate_utf8/1 should pass through both of those characters, so technically users could insert keys with those crazy values in them:
```erlang
<<16#FFFF/utf8>> = jiffy:decode(<<"\"\\uFFFF\"">>).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1379843635
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
For more details, see [how Erlang handles Unicode strings](https://www.erlang.org/doc/reference_manual/expressions#unicode-segments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1379838747
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
That is not enough. In addition to this, you must make sure that `Arg` is a well-formed UTF8 literal otherwise the combination will crash. Also, it seems `16#10FFFF` is a valid Unicode value although it does not have a symbol assigned. But that is the same for `16#FFFF`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "willholley (via GitHub)" <gi...@apache.org>.
willholley commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380597426
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
I changed it in response to https://github.com/apache/couchdb/pull/4810#issuecomment-1790609445
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "willholley (via GitHub)" <gi...@apache.org>.
willholley commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380629457
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
thanks @nickva @pgj. I pushed an update to use `U+FFFF` and fixed the unit test for invalid UTF8 strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#issuecomment-1791577531
Full CI tests passed https://ci-couchdb.apache.org/blue/organizations/jenkins/jenkins-cm1%2FFullPlatformMatrix/detail/jenkins-mango-begingswith-fixes-2/1/pipeline but see the minor test fix comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380459255
##########
src/mango/src/mango_selector.erl:
##########
@@ -1070,12 +1073,17 @@ check_beginswith(Field, Prefix) ->
match_beginswith_test() ->
% matching
?assertEqual(true, check_beginswith(<<"_id">>, <<"f">>)),
- % no match (user_id is not a binary string)
+ % no match (user_id field in the test doc contains an integer)
?assertEqual(false, check_beginswith(<<"user_id">>, <<"f">>)),
% invalid (prefix is not a binary string)
?assertThrow(
{mango_error, mango_selector, {invalid_operator, <<"$beginsWith">>}},
check_beginswith(<<"user_id">>, 1)
+ ),
+ % invalid (prefix is not a utf8 string)
+ ?assertThrow(
+ {mango_error, mango_selector, {invalid_operator, <<"$beginsWith">>}},
+ check_beginswith(<<123>>, 1)
Review Comment:
`<<123>>` is the field name here not the prefix. Is this a mistake...?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "willholley (via GitHub)" <gi...@apache.org>.
willholley merged PR #4828:
URL: https://github.com/apache/couchdb/pull/4828
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380675948
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
While my brain has proven itself resourceful enough in the recent decades (I am an arts guy who became an engineer because of the love creating things) my facial hair has not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380667886
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
All that's missing now is for you to grow a hipster beard 😜
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
All that's missing now is for you to grow a hipster beard 😜
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "willholley (via GitHub)" <gi...@apache.org>.
willholley commented on PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#issuecomment-1790827914
@pgj updated as per #4829
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "willholley (via GitHub)" <gi...@apache.org>.
willholley commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1379933225
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
yes - it's all a bit tricky ensuring that we don't double encode. I'll look at adding some tests for `range` directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380586593
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
Would `16#FFFF/utf8` work here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380608148
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
For the sake of diligence, I have launched another full CI run with `U+FFFF` in #4829.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "willholley (via GitHub)" <gi...@apache.org>.
willholley commented on PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#issuecomment-1790448696
@pgj I'm trying to think where it would make sense to add unit tests for non-UTF8 data. There are no existing eunit tests for `field_ranges` or `range` (perhaps we add some?) and I don't think it's possible to generate non-UTF8 data via the integration tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "willholley (via GitHub)" <gi...@apache.org>.
willholley commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380595300
##########
src/mango/src/mango_selector.erl:
##########
@@ -1070,12 +1073,17 @@ check_beginswith(Field, Prefix) ->
match_beginswith_test() ->
% matching
?assertEqual(true, check_beginswith(<<"_id">>, <<"f">>)),
- % no match (user_id is not a binary string)
+ % no match (user_id field in the test doc contains an integer)
?assertEqual(false, check_beginswith(<<"user_id">>, <<"f">>)),
% invalid (prefix is not a binary string)
?assertThrow(
{mango_error, mango_selector, {invalid_operator, <<"$beginsWith">>}},
check_beginswith(<<"user_id">>, 1)
+ ),
+ % invalid (prefix is not a utf8 string)
+ ?assertThrow(
+ {mango_error, mango_selector, {invalid_operator, <<"$beginsWith">>}},
+ check_beginswith(<<123>>, 1)
Review Comment:
my mistake - corrected
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380688095
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
> All that's missing now is for you to grow a hipster beard 😜
There is a unicode code point for it! 🧔 (`U+1F9D4`)
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
> All that's missing now is for you to grow a hipster beard 😜
There is a unicode code point for it! 🧔 (`U+1F9D4`)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380600671
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
Okay, thanks for the confirmation. Then `U+FFFF` is the constant to go.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380599542
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
I think `U+FFFF` is the canonical "highest sorting" string according to the collator rules, `U+10FFFF` is just the highest unicode code point numerically but not sure if is guaranteed to be the highest sorting one. Collators can be tailored in various ways and such.
Verifying that `U+FFFF` sorts higher than `U+10FFFF`
```erlang
(node1@127.0.0.1)80> mango_json:cmp(<<16#10FFFF/utf8>>, <<16#FFFF/utf8>>).
-1
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380811414
##########
src/mango/src/mango_selector.erl:
##########
@@ -1070,12 +1073,18 @@ check_beginswith(Field, Prefix) ->
match_beginswith_test() ->
% matching
?assertEqual(true, check_beginswith(<<"_id">>, <<"f">>)),
- % no match (user_id is not a binary string)
+ % no match (user_id field in the test doc contains an integer)
?assertEqual(false, check_beginswith(<<"user_id">>, <<"f">>)),
% invalid (prefix is not a binary string)
?assertThrow(
{mango_error, mango_selector, {invalid_operator, <<"$beginsWith">>}},
check_beginswith(<<"user_id">>, 1)
+ ),
+ % invalid (prefix is not a utf8 string)
+ InvalidArg = <<32#FFFFF>>,
Review Comment:
A tiny nit: this would just be `<<16#EF>>` as we're creating a base 32 larger integer, but only the last byte is used from it.
```erlang
<<16#EF>> =:= <<32#FFFFF>>.
true
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "big-r81 (via GitHub)" <gi...@apache.org>.
big-r81 commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380700798
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
🤣 that should go into one test 😉
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
🤣 that should go into one test 😉
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380599046
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
@nickva see [my comment](https://github.com/apache/couchdb/pull/4810#issuecomment-1790609445) on #4810:
_"Erlang itself supports `U+10FFFF` and it is numerically greater than `U+FFFF`"_ -- while I accept that `U+FFFF` might be the Unicode-official marker, Erlang's support for a higher range worries me a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380811414
##########
src/mango/src/mango_selector.erl:
##########
@@ -1070,12 +1073,18 @@ check_beginswith(Field, Prefix) ->
match_beginswith_test() ->
% matching
?assertEqual(true, check_beginswith(<<"_id">>, <<"f">>)),
- % no match (user_id is not a binary string)
+ % no match (user_id field in the test doc contains an integer)
?assertEqual(false, check_beginswith(<<"user_id">>, <<"f">>)),
% invalid (prefix is not a binary string)
?assertThrow(
{mango_error, mango_selector, {invalid_operator, <<"$beginsWith">>}},
check_beginswith(<<"user_id">>, 1)
+ ),
+ % invalid (prefix is not a utf8 string)
+ InvalidArg = <<32#FFFFF>>,
Review Comment:
A tiny nit: this would just be `<<16#EF>>` as is we're creating a base 32 larger integer, but only the last byte is used from it.
```erlang
<<16#EF>> =:= <<32#FFFFF>>.
true
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1381524075
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
Thanks @nickva !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#issuecomment-1790482735
If you implement the pre-check that I suggested then it can be established for `mango_idx_view:range/5` that the input will always be a valid UTF-8 input hence no tests are needed. The requirement for the test could be then pushed to the validation phase.
When I experimented with this fix locally, I extended `mango_selector:norm_ops/1` as follows:
```diff
diff --git a/src/mango/src/mango_selector.erl b/src/mango/src/mango_selector.erl
index 93d3b10ca..24660e963 100644
--- a/src/mango/src/mango_selector.erl
+++ b/src/mango/src/mango_selector.erl
@@ -136,7 +136,10 @@ norm_ops({[{<<"$text">>, Arg}]}) when
norm_ops({[{<<"$text">>, Arg}]}) ->
?MANGO_ERROR({bad_arg, '$text', Arg});
norm_ops({[{<<"$beginsWith">>, Arg}]} = Cond) when is_binary(Arg) ->
- Cond;
+ case couch_util:validate_utf8(Arg) of
+ true -> Cond;
+ false -> ?MANGO_ERROR({bad_arg, '$beginsWith', Arg})
+ end;
% Not technically an operator but we pass it through here
% so that this function accepts its own output. This exists
% so that $text can have a field name value which simplifies
```
With that, the existing definition of `match_beginswith_test/0` in the same module could be extended naturally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "willholley (via GitHub)" <gi...@apache.org>.
willholley commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380595004
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
@nickva that's what I used in the first iteration - is there a preference for `16#FFFF/utf8` over `16#10FFFF/utf8`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380599542
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
I think `U+FFFF` is the canonical "highest sorting" string according to the collator rules, `U+10FFFF` is just the highest unicode code point numerically but not sure if is guaranteed to be the highest sorting one. Collators can be tailed in various ways and such.
Verifying that `U+FFFF` sorts higher than `U+10FFFF`
```erlang
(node1@127.0.0.1)80> mango_json:cmp(<<16#10FFFF/utf8>>, <<16#FFFF/utf8>>).
-1
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380662149
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
Yes, that completely makes sense. In retrospect, it is quite logical that the collation algorithm may define arbitrary ordering over the values independently of their numerical representation. It may not be that efficient though.
I am somewhat old school in terms of character encodings (despite being a person for who the letters in the native language does not fit in 7-bit ASCII – ISO-8859-2 for the win! But I was able to do the move to IPv6 recently so there is still hope for me... :smile:)
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
Yes, that completely makes sense. In retrospect, it is quite logical that the collation algorithm may define arbitrary ordering over the values independently of their numerical representation. It may not be that efficient though.
I am somewhat old school in terms of character encodings (despite being a person for who the letters in the native language does not fit in 7-bit ASCII – ISO-8859-2 for the win! But I was able to do the move to IPv6 recently so there is still hope for me... :smile:)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] mango: fix $beginsWith range [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #4828:
URL: https://github.com/apache/couchdb/pull/4828#discussion_r1380652572
##########
src/mango/src/mango_idx_view.erl:
##########
@@ -417,7 +417,7 @@ range(_, _, LCmp, Low, HCmp, High) ->
% beginsWith requires both a high and low bound
range({[{<<"$beginsWith">>, Arg}]}, LCmp, Low, HCmp, High) ->
{LCmp0, Low0, HCmp0, High0} = range({[{<<"$gte">>, Arg}]}, LCmp, Low, HCmp, High),
- range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF>>}]}, LCmp0, Low0, HCmp0, High0);
+ range({[{<<"$lte">>, <<Arg/binary, 16#10FFFF/utf8>>}]}, LCmp0, Low0, HCmp0, High0);
Review Comment:
> "Erlang itself supports U+10FFFF and it is numerically greater than U+FFFF" -- while I accept that U+FFFF might be the Unicode-official marker, Erlang's support for a higher range worries me a bit.
Perhaps confusingly unicode collation rules don't sort by code point values. Higher code-points may sort lower. Besides just `U+10FFFF` and `U+FFFF` there are other such example:
```erlang
mango_json:cmp(<<16#1D175/utf8>>, <<16#41/utf8>>).
-1
```
Now, there may not be any other characters higher than U+10FFFF and lower than U+FFFF, so either one is probably ok in practice, but it's a bit neater if we can use the canonical max sorting character. Jiffy / erlang / couch_util:validate_utf8/1 should pass through both of those characters, so technically users could insert keys with those crazy values in them:
```erlang
<<16#FFFF/utf8>> = jiffy:decode(<<"\"\\uFFFF\"">>).
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org