From eb751d777f864d765872bfc653ca85298af1df70 Mon Sep 17 00:00:00 2001 From: Vlad Ilyushchenko Date: Wed, 13 Oct 2021 18:00:47 +0100 Subject: [PATCH] fix(sql): implemented bind variable for regex function. Fixed expr parser to report unclosed quotes (#1431) --- .gitignore | 1 + .../io/questdb/griffin/ExpressionParser.java | 4 +- .../java/io/questdb/griffin/SqlKeywords.java | 44 +------ .../regex/MatchStrFunctionFactory.java | 72 ++++++++++-- .../table/FilteredRecordCursorFactory.java | 10 +- .../io/questdb/cairo/AbstractCairoTest.java | 3 + .../questdb/griffin/AbstractGriffinTest.java | 4 +- .../questdb/griffin/ExpressionParserTest.java | 18 +++ .../griffin/WhereClauseParserTest.java | 2 +- .../bind/MatchStrBindVariableTest.java | 111 ++++++++++++++++++ .../java/io/questdb/test/tools/TestUtils.java | 2 +- 11 files changed, 214 insertions(+), 57 deletions(-) create mode 100644 core/src/test/java/io/questdb/griffin/engine/functions/bind/MatchStrBindVariableTest.java diff --git a/.gitignore b/.gitignore index 43b95d2a1..fa4e0b6b4 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ out benchmarks/target core/target +utils/target core/node core/src/main/resources/io/questdb/site/public.zip deploy*.bat diff --git a/core/src/main/java/io/questdb/griffin/ExpressionParser.java b/core/src/main/java/io/questdb/griffin/ExpressionParser.java index 486b0a323..39e259f18 100644 --- a/core/src/main/java/io/questdb/griffin/ExpressionParser.java +++ b/core/src/main/java/io/questdb/griffin/ExpressionParser.java @@ -25,7 +25,6 @@ package io.questdb.griffin; import io.questdb.cairo.ColumnType; -import io.questdb.cairo.GeoHashes; import io.questdb.griffin.model.ExpressionNode; import io.questdb.std.*; @@ -666,6 +665,9 @@ class ExpressionParser { } } if (prevBranch != BRANCH_DOT && nonLiteralBranches.excludes(prevBranch)) { + if (SqlKeywords.isQuote(tok)) { + throw SqlException.$(position, "unclosed quoted string?"); + } opStack.push(expressionNodePool.next().of(ExpressionNode.CONSTANT, GenericLexer.immutableOf(tok), 0, position)); break; } else { diff --git a/core/src/main/java/io/questdb/griffin/SqlKeywords.java b/core/src/main/java/io/questdb/griffin/SqlKeywords.java index c555665bb..1db737ef2 100644 --- a/core/src/main/java/io/questdb/griffin/SqlKeywords.java +++ b/core/src/main/java/io/questdb/griffin/SqlKeywords.java @@ -578,6 +578,10 @@ public class SqlKeywords { return isGeoHashKeyword(tok, i); } + public static boolean isQuote(CharSequence tok) { + return tok.length() == 1 && tok.charAt(0) == '\''; + } + private static boolean isGeoHashKeyword(CharSequence tok, int i) { return (tok.charAt(i++) | 32) == 'g' && (tok.charAt(i++) | 32) == 'e' @@ -821,20 +825,6 @@ public class SqlKeywords { && (tok.charAt(i) | 32) == 'k'; } - public static boolean isMasterKeyword(CharSequence tok) { - if (tok.length() != 6) { - return false; - } - - int i = 0; - return (tok.charAt(i++) | 32) == 'm' - && (tok.charAt(i++) | 32) == 'a' - && (tok.charAt(i++) | 32) == 's' - && (tok.charAt(i++) | 32) == 't' - && (tok.charAt(i++) | 32) == 'e' - && (tok.charAt(i) | 32) == 'r'; - } - public static boolean isMaxUncommittedRowsParam(CharSequence tok) { if (tok.length() != 18) { return false; @@ -1202,19 +1192,6 @@ public class SqlKeywords { && (tok.charAt(i) | 32) == 't'; } - public static boolean isSlaveKeyword(CharSequence tok) { - if (tok.length() != 5) { - return false; - } - - int i = 0; - return (tok.charAt(i++) | 32) == 's' - && (tok.charAt(i++) | 32) == 'l' - && (tok.charAt(i++) | 32) == 'a' - && (tok.charAt(i++) | 32) == 'v' - && (tok.charAt(i) | 32) == 'e'; - } - public static boolean isStandardConformingStringsKeyword(CharSequence tok) { if (tok.length() != 27) { return false; @@ -1250,19 +1227,6 @@ public class SqlKeywords { && (tok.charAt(i) | 32) == 's'; } - public static boolean isStartKeyword(CharSequence tok) { - if (tok.length() != 5) { - return false; - } - - int i = 0; - return (tok.charAt(i++) | 32) == 's' - && (tok.charAt(i++) | 32) == 't' - && (tok.charAt(i++) | 32) == 'a' - && (tok.charAt(i++) | 32) == 'r' - && (tok.charAt(i) | 32) == 't'; - } - public static boolean isSumKeyword(CharSequence tok) { if (tok.length() != 3) { return false; diff --git a/core/src/main/java/io/questdb/griffin/engine/functions/regex/MatchStrFunctionFactory.java b/core/src/main/java/io/questdb/griffin/engine/functions/regex/MatchStrFunctionFactory.java index 5508667f8..4843562be 100644 --- a/core/src/main/java/io/questdb/griffin/engine/functions/regex/MatchStrFunctionFactory.java +++ b/core/src/main/java/io/questdb/griffin/engine/functions/regex/MatchStrFunctionFactory.java @@ -27,6 +27,7 @@ package io.questdb.griffin.engine.functions.regex; import io.questdb.cairo.CairoConfiguration; import io.questdb.cairo.sql.Function; import io.questdb.cairo.sql.Record; +import io.questdb.cairo.sql.SymbolTableSource; import io.questdb.griffin.FunctionFactory; import io.questdb.griffin.SqlException; import io.questdb.griffin.SqlExecutionContext; @@ -35,6 +36,7 @@ import io.questdb.griffin.engine.functions.UnaryFunction; import io.questdb.std.Chars; import io.questdb.std.IntList; import io.questdb.std.ObjList; +import org.jetbrains.annotations.NotNull; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -43,7 +45,7 @@ import java.util.regex.PatternSyntaxException; public class MatchStrFunctionFactory implements FunctionFactory { @Override public String getSignature() { - return "~(Ss)"; + return "~(SS)"; } @Override @@ -54,39 +56,89 @@ public class MatchStrFunctionFactory implements FunctionFactory { CairoConfiguration configuration, SqlExecutionContext sqlExecutionContext ) throws SqlException { - Function value = args.getQuick(0); - CharSequence regex = args.getQuick(1).getStr(null); + final Function value = args.getQuick(0); + final Function pattern = args.getQuick(1); + final int patternPosition = argPositions.getQuick(1); + if (pattern.isConstant()) { + return new MatchConstPatternFunction(value, createMatcher(pattern, patternPosition)); + } else if (pattern.isRuntimeConstant()) { + return new MatchRuntimeConstPatternFunction(value, pattern, patternPosition); + } + throw SqlException.$(patternPosition, "not implemented: dynamic patter would be very slow to execute"); + } + @NotNull + private static Matcher createMatcher(Function pattern, int position) throws SqlException { + final CharSequence regex = pattern.getStr(null); if (regex == null) { - throw SqlException.$(argPositions.getQuick(1), "NULL regex"); + throw SqlException.$(position, "NULL regex"); } - try { - Matcher matcher = Pattern.compile(Chars.toString(regex)).matcher(""); - return new MatchFunction(value, matcher); + return Pattern.compile(Chars.toString(regex)).matcher(""); } catch (PatternSyntaxException e) { - throw SqlException.$(argPositions.getQuick(1) + e.getIndex() + 1, e.getMessage()); + throw SqlException.$(position + e.getIndex() + 1, e.getMessage()); } } - private static class MatchFunction extends BooleanFunction implements UnaryFunction { + private static class MatchConstPatternFunction extends BooleanFunction implements UnaryFunction { private final Function value; private final Matcher matcher; - public MatchFunction(Function value, Matcher matcher) { + public MatchConstPatternFunction(Function value, Matcher matcher) { this.value = value; this.matcher = matcher; } + @Override + public Function getArg() { + return value; + } + @Override public boolean getBool(Record rec) { CharSequence cs = getArg().getStr(rec); return cs != null && matcher.reset(cs).find(); } + } + + private static class MatchRuntimeConstPatternFunction extends BooleanFunction implements UnaryFunction { + private final Function value; + private final Function pattern; + private final int patternPosition; + private Matcher matcher; + + public MatchRuntimeConstPatternFunction(Function value, Function pattern, int patternPosition) { + this.value = value; + this.pattern = pattern; + this.patternPosition = patternPosition; + } @Override public Function getArg() { return value; } + + @Override + public boolean getBool(Record rec) { + CharSequence cs = getArg().getStr(rec); + return cs != null && matcher.reset(cs).find(); + } + + @Override + public boolean isConstant() { + return false; + } + + @Override + public boolean isRuntimeConstant() { + return false; + } + + @Override + public void init(SymbolTableSource symbolTableSource, SqlExecutionContext executionContext) throws SqlException { + UnaryFunction.super.init(symbolTableSource, executionContext); + pattern.init(symbolTableSource, executionContext); + this.matcher = createMatcher(pattern, patternPosition); + } } } diff --git a/core/src/main/java/io/questdb/griffin/engine/table/FilteredRecordCursorFactory.java b/core/src/main/java/io/questdb/griffin/engine/table/FilteredRecordCursorFactory.java index f5ae65d37..381f098c1 100644 --- a/core/src/main/java/io/questdb/griffin/engine/table/FilteredRecordCursorFactory.java +++ b/core/src/main/java/io/questdb/griffin/engine/table/FilteredRecordCursorFactory.java @@ -30,6 +30,7 @@ import io.questdb.cairo.sql.RecordCursorFactory; import io.questdb.cairo.sql.RecordMetadata; import io.questdb.griffin.SqlException; import io.questdb.griffin.SqlExecutionContext; +import io.questdb.std.Misc; public class FilteredRecordCursorFactory implements RecordCursorFactory { private final RecordCursorFactory base; @@ -52,8 +53,13 @@ public class FilteredRecordCursorFactory implements RecordCursorFactory { @Override public RecordCursor getCursor(SqlExecutionContext executionContext) throws SqlException { RecordCursor cursor = base.getCursor(executionContext); - this.cursor.of(cursor, executionContext); - return this.cursor; + try { + this.cursor.of(cursor, executionContext); + return this.cursor; + } catch (Throwable e) { + Misc.free(cursor); + throw e; + } } @Override diff --git a/core/src/test/java/io/questdb/cairo/AbstractCairoTest.java b/core/src/test/java/io/questdb/cairo/AbstractCairoTest.java index 3820343ee..b7f302c13 100644 --- a/core/src/test/java/io/questdb/cairo/AbstractCairoTest.java +++ b/core/src/test/java/io/questdb/cairo/AbstractCairoTest.java @@ -28,9 +28,11 @@ import io.questdb.MessageBus; import io.questdb.Metrics; import io.questdb.cairo.sql.RecordCursor; import io.questdb.cairo.sql.RecordMetadata; +import io.questdb.griffin.engine.functions.rnd.SharedRandom; import io.questdb.log.Log; import io.questdb.log.LogFactory; import io.questdb.std.FilesFacade; +import io.questdb.std.Rnd; import io.questdb.std.datetime.microtime.MicrosecondClock; import io.questdb.std.datetime.microtime.MicrosecondClockImpl; import io.questdb.std.str.StringSink; @@ -149,6 +151,7 @@ public class AbstractCairoTest { TestUtils.createTestPath(root); engine.openTableId(); engine.resetTableId(); + SharedRandom.RANDOM.set(new Rnd()); } @After diff --git a/core/src/test/java/io/questdb/griffin/AbstractGriffinTest.java b/core/src/test/java/io/questdb/griffin/AbstractGriffinTest.java index d8a315e04..ffc588656 100644 --- a/core/src/test/java/io/questdb/griffin/AbstractGriffinTest.java +++ b/core/src/test/java/io/questdb/griffin/AbstractGriffinTest.java @@ -1013,7 +1013,7 @@ public class AbstractGriffinTest extends AbstractCairoTest { SqlExecutionContext sqlExecutionContext, boolean supportsRandomAccess, boolean checkSameStr, - boolean expectSize ) throws SqlException { + boolean expectSize) throws SqlException { try (final RecordCursorFactory factory = compiler.compile(query, sqlExecutionContext).getRecordCursorFactory()) { assertFactoryCursor( expected, @@ -1035,7 +1035,7 @@ public class AbstractGriffinTest extends AbstractCairoTest { boolean supportsRandomAccess, boolean checkSameStr, boolean expectSize, - boolean sizeCanBeVariable ) throws SqlException { + boolean sizeCanBeVariable) throws SqlException { try (final RecordCursorFactory factory = compiler.compile(query, sqlExecutionContext).getRecordCursorFactory()) { assertFactoryCursor( expected, diff --git a/core/src/test/java/io/questdb/griffin/ExpressionParserTest.java b/core/src/test/java/io/questdb/griffin/ExpressionParserTest.java index cc6c40e9d..5a43ca8e5 100644 --- a/core/src/test/java/io/questdb/griffin/ExpressionParserTest.java +++ b/core/src/test/java/io/questdb/griffin/ExpressionParserTest.java @@ -115,6 +115,24 @@ public class ExpressionParserTest extends AbstractCairoTest { ); } + @Test + public void testUnquotedRegexFail() { + assertFail( + "s ~ '.*TDF", + 4, + "unclosed quoted string?" + ); + } + + @Test + public void testUnquotedStrFail() { + assertFail( + "s ~ 'TDF", + 4, + "unclosed quoted string?" + ); + } + @Test public void testAllNoOperator() { assertFail( diff --git a/core/src/test/java/io/questdb/griffin/WhereClauseParserTest.java b/core/src/test/java/io/questdb/griffin/WhereClauseParserTest.java index 4ea101db8..7cca241c5 100644 --- a/core/src/test/java/io/questdb/griffin/WhereClauseParserTest.java +++ b/core/src/test/java/io/questdb/griffin/WhereClauseParserTest.java @@ -288,7 +288,7 @@ public class WhereClauseParserTest extends AbstractCairoTest { modelOf("invalidTimestamp between '2014-01-01T12:30:00.000Z' and '2014-01-02T12:30:00.000Z"); Assert.fail(); } catch (SqlException e) { - TestUtils.assertContains(e.getFlyweightMessage(), "dangling expression"); + TestUtils.assertContains(e.getFlyweightMessage(), "unclosed quoted string?"); } } diff --git a/core/src/test/java/io/questdb/griffin/engine/functions/bind/MatchStrBindVariableTest.java b/core/src/test/java/io/questdb/griffin/engine/functions/bind/MatchStrBindVariableTest.java new file mode 100644 index 000000000..6015d5a48 --- /dev/null +++ b/core/src/test/java/io/questdb/griffin/engine/functions/bind/MatchStrBindVariableTest.java @@ -0,0 +1,111 @@ +/******************************************************************************* + * ___ _ ____ ____ + * / _ \ _ _ ___ ___| |_| _ \| __ ) + * | | | | | | |/ _ \/ __| __| | | | _ \ + * | |_| | |_| | __/\__ \ |_| |_| | |_) | + * \__\_\\__,_|\___||___/\__|____/|____/ + * + * Copyright (c) 2014-2019 Appsicle + * Copyright (c) 2019-2020 QuestDB + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************************/ + +package io.questdb.griffin.engine.functions.bind; + +import io.questdb.cairo.sql.RecordCursor; +import io.questdb.cairo.sql.RecordCursorFactory; +import io.questdb.griffin.AbstractGriffinTest; +import io.questdb.griffin.SqlException; +import io.questdb.test.tools.TestUtils; +import org.junit.Assert; +import org.junit.Test; + +public class MatchStrBindVariableTest extends AbstractGriffinTest { + + @Test + public void testSimple() throws Exception { + assertMemoryLeak(() -> { + compiler.compile("create table x as (select rnd_str() s from long_sequence(100))", sqlExecutionContext); + + try (RecordCursorFactory factory = compiler.compile("x where s ~ $1", sqlExecutionContext).getRecordCursorFactory()) { + bindVariableService.setStr(0, "GQO"); + try (RecordCursor cursor = factory.getCursor(sqlExecutionContext)) { + TestUtils.printCursor(cursor, factory.getMetadata(), true, sink, TestUtils.printer); + } + + TestUtils.assertEquals("s\n" + + "YCTGQO\n", sink); + + bindVariableService.setStr(0, "QTQ"); + try (RecordCursor cursor = factory.getCursor(sqlExecutionContext)) { + TestUtils.printCursor(cursor, factory.getMetadata(), true, sink, TestUtils.printer); + } + + TestUtils.assertEquals("s\n" + + "ZWEVQTQO\n", sink); + + bindVariableService.setStr(0, null); + try { + factory.getCursor(sqlExecutionContext); + Assert.fail(); + } catch (SqlException e) { + Assert.assertEquals(12, e.getPosition()); + TestUtils.assertContains(e.getFlyweightMessage(), "NULL regex"); + } + } + }); + } + + @Test + public void testConstant() throws Exception { + assertMemoryLeak(() -> { + try (RecordCursorFactory factory = compiler.compile("select x from long_sequence(1) where '1GQO2' ~ $1", sqlExecutionContext).getRecordCursorFactory()) { + bindVariableService.setStr(0, "GQO"); + try (RecordCursor cursor = factory.getCursor(sqlExecutionContext)) { + TestUtils.printCursor(cursor, factory.getMetadata(), true, sink, TestUtils.printer); + } + + TestUtils.assertEquals("x\n" + + "1\n", sink); + + bindVariableService.setStr(0, "QTQ"); + try (RecordCursor cursor = factory.getCursor(sqlExecutionContext)) { + TestUtils.printCursor(cursor, factory.getMetadata(), true, sink, TestUtils.printer); + } + + TestUtils.assertEquals("x\n", sink); + + bindVariableService.setStr(0, null); + try { + factory.getCursor(sqlExecutionContext); + Assert.fail(); + } catch (SqlException e) { + Assert.assertEquals(47, e.getPosition()); + TestUtils.assertContains(e.getFlyweightMessage(), "NULL regex"); + } + } + }); + } + + @Test + public void testDynamicRegexFailure() throws Exception { + assertFailure( + "x where s ~ s", + "create table x as (select rnd_str() s from long_sequence(100))", + 12, + "not implemented: dynamic patter would be very slow to execute" + ); + } +} diff --git a/core/src/test/java/io/questdb/test/tools/TestUtils.java b/core/src/test/java/io/questdb/test/tools/TestUtils.java index 3637c48b6..933db71e8 100644 --- a/core/src/test/java/io/questdb/test/tools/TestUtils.java +++ b/core/src/test/java/io/questdb/test/tools/TestUtils.java @@ -48,7 +48,7 @@ import java.util.concurrent.atomic.AtomicInteger; public final class TestUtils { - private static final RecordCursorPrinter printer = new RecordCursorPrinter(); + public static final RecordCursorPrinter printer = new RecordCursorPrinter(); private static final RecordCursorPrinter printerWithTypes = new RecordCursorPrinter().withTypes(true); -- GitLab