Address review comments

python/lsst/daf/butler/exprParser/exprTree.py

@@ -214,10 +214,13 @@ class RangeLiteral(Node):
         Start value of a range.
     stop : `int`
         End value of a range, inclusive, same or higher than ``start``.
-    stride : `int` or `None`
-        Stride value, must be positive, can be `None`.
+    stride : `int` or `None`, optional
+        Stride value, must be positive, can be `None` which means that stride
+        was not specified. Consumers are supposed to treat `None` the same way
+        as stride=1 but for some consumers it may be useful to know that
+        stride was missing from literal.
     """
-    def __init__(self, start, stop, stride=1):
+    def __init__(self, start, stop, stride=None):
         self.start = start
         self.stop = stop
         self.stride = stride
@@ -227,9 +230,7 @@ def visit(self, visitor):
         return visitor.visitRangeLiteral(self.start, self.stop, self.stride, self)
 
     def __str__(self):
-        res = f"{self.start}..{self.stop}"
-        if self.stride is not None:
-            res += f":{self.stride}"
+        res = f"{self.start}..{self.stop}" + (f":{self.stride}" if self.stride else "")
         return res
 
 

python/lsst/daf/butler/exprParser/parserLex.py

@@ -38,7 +38,7 @@
 #  Local non-exported definitions --
 # ----------------------------------
 
-_RE_RANGE = re.compile(r"(-?\d+)\s*\.\.\s*(-?\d+)(\s*:\s*(\d+))?")
+_RE_RANGE = r"(?P<start>-?\d+)\s*\.\.\s*(?P<stop>-?\d+)(\s*:\s*(?P<stride>[1-9]\d*))?"
 """Regular expression to match range literal in the form NUM..NUM[:NUM],
 this must match t_RANGE_LITERAL docstring.
 """
@@ -58,7 +58,7 @@ class ParserLexError(Exception):
     remain : str
         Remaining non-parsed part of the expression
     pos : int
-        Current parsing posistion, offset from beginning of expression in
+        Current parsing position, offset from beginning of expression in
         characters
     lineno : int
         Current line number in the expression
@@ -154,15 +154,13 @@ def t_STRING_LITERAL(self, t):
         t.value = t.value[1:-1]
         return t
 
-    # range literal in format N..M[:S], spaces allowed
-    # docstring must match _RE_RANGE regexp
+    # range literal in format N..M[:S], spaces allowed, see _RE_RANGE
+    @lex.TOKEN(_RE_RANGE)
     def t_RANGE_LITERAL(self, t):
-        r"""(-?\d+)\s*\.\.\s*(-?\d+)(\s*:\s*(\d+))?
-        """
-        match = _RE_RANGE.match(t.value)
-        start = int(match.group(1))
-        stop = int(match.group(2))
-        stride = match.group(4)
+        match = re.match(_RE_RANGE, t.value)
+        start = int(match.group("start"))
+        stop = int(match.group("stop"))
+        stride = match.group("stride")
         if stride is not None:
             stride = int(stride)
         t.value = (start, stop, stride)

python/lsst/daf/butler/exprParser/parserYacc.py

@@ -259,7 +259,9 @@ def p_literal_str(self, p):
     def p_literal_range(self, p):
         """ literal : RANGE_LITERAL
         """
-        p[0] = RangeLiteral(*p[1])
+        # RANGE_LITERAL value is tuple of three numbers
+        start, stop, stride = p[1]
+        p[0] = RangeLiteral(start, stop, stride)
 
     # ---------- end of all grammar rules ----------
 

python/lsst/daf/butler/exprParser/treeVisitor.py

@@ -72,7 +72,7 @@ def visitRangeLiteral(self, start, stop, stride, node):
             Range starting value.
         stop : `int`
             Range final value.
-        stride : `int`
+        stride : `int` or `None`
             Stride, can be `None` if not specified (should be treated same
             as 1).
         node : `Node`

python/lsst/daf/butler/sql/queryBuilder.py

@@ -81,10 +81,9 @@ def visitStringLiteral(self, value, node):
     def visitRangeLiteral(self, start, stop, stride, node):
         # Docstring inherited from TreeVisitor.visitRangeLiteral
 
-        # just return a triple and let parent clauses handle it
-        if stride is None:
-            stride = 1
-        return (start, stop, stride)
+        # Just return a triple and let parent clauses handle it,
+        # stride can be None which means the same as 1
+        return (start, stop, stride or 1)
 
     def visitIdentifier(self, name, node):
         # Docstring inherited from TreeVisitor.visitIdentifier
@@ -136,9 +135,10 @@ def visitIsIn(self, lhs, values, not_in, node):
         #
         # or for NOT IN case
         #
-        #    X NOT IN (1, 2, 3)
-        #    AND NOT
-        #    (X BETWEEN START AND STOP AND MOD(X, STRIDE) = MOD(START, STRIDE))
+        #    NOT (X IN (1, 2, 3)
+        #         OR
+        #         (X BETWEEN START AND STOP
+        #          AND MOD(X, STRIDE) = MOD(START, STRIDE)))
 
         max_in_items = 1000
 
@@ -150,28 +150,29 @@ def visitIsIn(self, lhs, values, not_in, node):
             else:
                 literals.append(item)
 
-        ranges_remain = []
+        clauses = []
         for start, stop, stride in ranges:
             count = (stop - start + 1) // stride
             if len(literals) + count > max_in_items:
-                ranges_remain.append((start, stop, stride))
+                # X BETWEEN START AND STOP
+                #    AND MOD(X, STRIDE) = MOD(START, STRIDE)
+                expr = lhs.between(start, stop)
+                if stride != 1:
+                    expr = and_(expr, (lhs % stride) == (start % stride))
+                clauses.append(expr)
             else:
                 # add all values to literal list, stop is inclusive
                 literals += [literal(value) for value in range(start, stop+1, stride)]
 
-        clauses = [lhs.in_(literals)]
-        for start, stop, stride in ranges_remain:
-            # X BETWEEN START AND STOP AND MOD(X, STRIDE) = MOD(START, STRIDE)
-            expr = lhs.between(start, stop)
-            if stride != 1:
-                expr = and_(expr, (lhs % stride) == (start % stride))
-            clauses.append(expr)
+        if literals:
+            # add IN() in front of BETWEENs
+            clauses.insert(0, lhs.in_(literals))
 
+        expr = or_(*clauses)
         if not_in:
-            clauses = [not_(expr) for expr in clauses]
-            return and_(*clauses)
-        else:
-            return or_(*clauses)
+            expr = not_(expr)
+
+        return expr
 
     def visitParens(self, expression, node):
         # Docstring inherited from TreeVisitor.visitParens

tests/test_exprParserLex.py

@@ -272,6 +272,24 @@ def _assertExc(exc, expr, remain, pos, lineno):
             lexer.token()
         _assertExc(catcher.exception, expr, ".e2", 7, 3)
 
+        # zero stride in range literal
+        lexer = ParserLex.make_lexer()
+        expr = "1..2:0"
+        lexer.input(expr)
+        self._assertToken(lexer.token(), "RANGE_LITERAL", (1, 2, None))
+        with self.assertRaises(ParserLexError) as catcher:
+            lexer.token()
+        _assertExc(catcher.exception, expr, ":0", 4, 1)
+
+        # negative stride in range literal
+        lexer = ParserLex.make_lexer()
+        expr = "1..2:-10"
+        lexer.input(expr)
+        self._assertToken(lexer.token(), "RANGE_LITERAL", (1, 2, None))
+        with self.assertRaises(ParserLexError) as catcher:
+            lexer.token()
+        _assertExc(catcher.exception, expr, ":-10", 4, 1)
+
 
 if __name__ == "__main__":
     unittest.main()