Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
gerlowskija merged PR #4229: URL: https://github.com/apache/solr/pull/4229 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
gerlowskija commented on code in PR #4229:
URL: https://github.com/apache/solr/pull/4229#discussion_r2981901205
##
solr/core/src/java/org/apache/solr/search/numericrange/NumericRangeQParserPlugin.java:
##
@@ -69,13 +71,18 @@
* {!numericRange criteria="intersects" field=long_range}[10 TO
20]
* {!numericRange criteria="within" field=long_range}[0 TO 99]
*
+ * // FloatRangeField queries
+ * {!numericRange criteria="intersects" field=float_range}[1.0 TO 2.5]
+ * {!numericRange criteria="within" field=float_range}[0.0 TO 9.99]
+ *
* // Multi-dimensional queries (bounding boxes, cubes, tesseracts)
* {!numericRange criteria="intersects" field=bbox}[0,0 TO 10,10]
Review Comment:
Done!
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
gerlowskija commented on code in PR #4229:
URL: https://github.com/apache/solr/pull/4229#discussion_r2981815316
##
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##
@@ -82,9 +83,54 @@ public interface NumericRangeValue {
protected static final Pattern SINGLE_BOUND_PATTERN =
Pattern.compile("^" + COMMA_DELIMITED_NUMS + "$");
+ /**
+ * Regex fragment matching a comma-separated list of signed floating-point
numbers (integers or
+ * floating-point literals).
+ */
+ protected static final String COMMA_DELIMITED_FP_NUMS =
+ "-?\\d+(?:\\.\\d+)?(?:\\s*,\\s*-?\\d+(?:\\.\\d+)?)*";
+
+ private static final String FP_RANGE_PATTERN_STR =
Review Comment:
It was intentional, but I'm starting to second guess that decision now. My
assumption at the time was that our existing float and double fields (e.g.
DoublePointField) also didn't support scientific notation. There's no mention
of that in the ref-guide afaict at least.
But in testing it just now I can see that they do actually support it.
Thanks for raising this - will fix.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
chan-dx commented on code in PR #4229:
URL: https://github.com/apache/solr/pull/4229#discussion_r2974673760
##
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##
@@ -82,9 +83,54 @@ public interface NumericRangeValue {
protected static final Pattern SINGLE_BOUND_PATTERN =
Pattern.compile("^" + COMMA_DELIMITED_NUMS + "$");
+ /**
+ * Regex fragment matching a comma-separated list of signed floating-point
numbers (integers or
+ * floating-point literals).
+ */
+ protected static final String COMMA_DELIMITED_FP_NUMS =
Review Comment:
Your current approach is already really clean IMO, since it keeps validation
in one place.
I did consider a more type agnostic two-step approach: use just one regex to
validate the overall` [... TO ...] `structure to reduce some of the regex
complexity, then let `parseFloatArray` in `FloatRangeField.java` reject invalid
values downstream. But mismatched-dimension edge cases would need separate
handling, so that ends up splitting the validation rather than really
simplifying it, which also doesn't align with your rationale.
- One small cleanup idea: since `RANGE_PATTERN_STR` and
`FP_RANGE_PATTERN_STR` share the same overall structure and mainly differ in
the numeric regex fragment they embedded, would it be worth introducing a small
helper like `buildRangePattern(String numericFragment)` to centralise the range
pattern assembly, and maybe even the compilation step as well? That might
reduce the duplication a little.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
chan-dx commented on code in PR #4229:
URL: https://github.com/apache/solr/pull/4229#discussion_r2969819393
##
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##
@@ -82,9 +83,54 @@ public interface NumericRangeValue {
protected static final Pattern SINGLE_BOUND_PATTERN =
Pattern.compile("^" + COMMA_DELIMITED_NUMS + "$");
+ /**
+ * Regex fragment matching a comma-separated list of signed floating-point
numbers (integers or
+ * floating-point literals).
+ */
+ protected static final String COMMA_DELIMITED_FP_NUMS =
+ "-?\\d+(?:\\.\\d+)?(?:\\s*,\\s*-?\\d+(?:\\.\\d+)?)*";
+
+ private static final String FP_RANGE_PATTERN_STR =
Review Comment:
I noticed a small gap in the float validation regex: it doesn't accept
scientific notation. I tested `FP_RANGE_PATTERN_STR` pattern on regex101.com,
and an input like `[1.1 TO 1.5e10] `does not match.
Was the exclusion of scientific notation intentional? Or are values like
this expected to be rejected before reaching this validation step?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
chan-dx commented on code in PR #4229:
URL: https://github.com/apache/solr/pull/4229#discussion_r2969819393
##
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##
@@ -82,9 +83,54 @@ public interface NumericRangeValue {
protected static final Pattern SINGLE_BOUND_PATTERN =
Pattern.compile("^" + COMMA_DELIMITED_NUMS + "$");
+ /**
+ * Regex fragment matching a comma-separated list of signed floating-point
numbers (integers or
+ * floating-point literals).
+ */
+ protected static final String COMMA_DELIMITED_FP_NUMS =
+ "-?\\d+(?:\\.\\d+)?(?:\\s*,\\s*-?\\d+(?:\\.\\d+)?)*";
+
+ private static final String FP_RANGE_PATTERN_STR =
Review Comment:
I noticed a small gap in the float validation regex: it doesn't accept
scientific notation. I tested this pattern on regex101.com, and an input like
`[1.1 TO 1.5e10] `does not match.
Was the exclusion of scientific notation intentional? Or are values like
this expected to be rejected before reaching this validation step?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
chan-dx commented on code in PR #4229:
URL: https://github.com/apache/solr/pull/4229#discussion_r2969819393
##
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##
@@ -82,9 +83,54 @@ public interface NumericRangeValue {
protected static final Pattern SINGLE_BOUND_PATTERN =
Pattern.compile("^" + COMMA_DELIMITED_NUMS + "$");
+ /**
+ * Regex fragment matching a comma-separated list of signed floating-point
numbers (integers or
+ * floating-point literals).
+ */
+ protected static final String COMMA_DELIMITED_FP_NUMS =
+ "-?\\d+(?:\\.\\d+)?(?:\\s*,\\s*-?\\d+(?:\\.\\d+)?)*";
+
+ private static final String FP_RANGE_PATTERN_STR =
Review Comment:
I noticed a small gap in the float validation regex: it doesn't accept
scientific notation. I tested this pattern on regex101.com, and an input like
`[-1 TO 1.5e10] `does not match.
Was the exclusion of scientific notation intentional? Or are values like
this expected to be rejected before reaching this validation step?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
HoustonPutman commented on code in PR #4229:
URL: https://github.com/apache/solr/pull/4229#discussion_r2967841069
##
solr/core/src/java/org/apache/solr/search/numericrange/NumericRangeQParserPlugin.java:
##
@@ -69,13 +71,18 @@
* {!numericRange criteria="intersects" field=long_range}[10 TO
20]
* {!numericRange criteria="within" field=long_range}[0 TO 99]
*
+ * // FloatRangeField queries
+ * {!numericRange criteria="intersects" field=float_range}[1.0 TO 2.5]
+ * {!numericRange criteria="within" field=float_range}[0.0 TO 9.99]
+ *
* // Multi-dimensional queries (bounding boxes, cubes, tesseracts)
* {!numericRange criteria="intersects" field=bbox}[0,0 TO 10,10]
Review Comment:
should `bbox` be renamed here? Might be confusing since we have a bbox field
type not supported here, right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] SOLR-13309: Introduce FloatRangeField to expose Lucene 'FloatRange' [solr]
gerlowskija commented on code in PR #4229:
URL: https://github.com/apache/solr/pull/4229#discussion_r2966735285
##
solr/core/src/java/org/apache/solr/schema/numericrange/AbstractNumericRangeField.java:
##
@@ -82,9 +83,54 @@ public interface NumericRangeValue {
protected static final Pattern SINGLE_BOUND_PATTERN =
Pattern.compile("^" + COMMA_DELIMITED_NUMS + "$");
+ /**
+ * Regex fragment matching a comma-separated list of signed floating-point
numbers (integers or
+ * floating-point literals).
+ */
+ protected static final String COMMA_DELIMITED_FP_NUMS =
Review Comment:
[0] All of these regexes are a bit messy.
Ultimately the idea here is that we want the validation for each
implementing type (int, float, etc.) to be specific to what that type looks
like. IntRangeField needs to be able to reject fp-values, etc.
This validation is done by regex. The regexes live in this base class
because some code here that invotes this verification. Individual sub-classes
specify the regex pattern they want to use by using overrideable methods
`getRangePattern` and `getSingleBoundPattern`.
So that's my rationale here. Open to other ways of doing it if folks can
see a better approach
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
