From 4bc15fc4ba625d23249fd5ae92b89898f5a6350f Mon Sep 17 00:00:00 2001 From: Eric Wang Date: Thu, 30 Apr 2026 16:03:56 -0700 Subject: [PATCH 1/2] fix(csharp): validate BatchSize/PollTime/QueryTimeout options in StatementExecutionStatement (PECO-3011) Co-Authored-By: Claude Sonnet 4.6 --- .../StatementExecutionStatement.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/csharp/src/StatementExecution/StatementExecutionStatement.cs b/csharp/src/StatementExecution/StatementExecutionStatement.cs index 466222185..5a01bfc18 100644 --- a/csharp/src/StatementExecution/StatementExecutionStatement.cs +++ b/csharp/src/StatementExecution/StatementExecutionStatement.cs @@ -175,12 +175,21 @@ public override void SetOption(string key, string value) _escapePatternWildcards = bool.TryParse(value, out bool escape) && escape; break; - // These options are readonly in SEA (set at connection level). - // Accept but ignore them to avoid NotImplemented exceptions for compatibility. + // Validate these options to match Thrift path behavior (throw ArgumentOutOfRangeException + // for invalid values), even though they are read-only in SEA (set at connection level). case ApacheParameters.PollTimeMilliseconds: + if (string.IsNullOrEmpty(value) || !int.TryParse(value, out int pollTimeMs) || pollTimeMs < 0) + throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value greater than or equal to 0."); + break; case ApacheParameters.BatchSize: + if (string.IsNullOrEmpty(value) || !long.TryParse(value, out long batchSize) || batchSize <= 0) + throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value greater than zero."); + break; case ApacheParameters.BatchSizeStopCondition: + break; case ApacheParameters.QueryTimeoutSeconds: + if (string.IsNullOrEmpty(value) || !int.TryParse(value, out int queryTimeoutSecs) || queryTimeoutSecs < 0) + throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value of 0 (infinite) or greater."); break; // DatabricksStatement-specific options: accept but ignore for now. From c57bdba51e45fe53bf8be179d6c1537996890cfb Mon Sep 17 00:00:00 2001 From: Eric Wang Date: Sun, 31 May 2026 23:44:01 -0700 Subject: [PATCH 2/2] fix(csharp): validate SEA statement options at construction too, close PECO-3011 PR #431 added SetOption validation but the inherited CanSetOption{PollTime,BatchSize, QueryTimeout} tests also assert that bad config throws ArgumentOutOfRangeException at NewConnection(...).CreateStatement(). SEA only validated in SetOption and its connection-level parsers threw plain ArgumentException for non-numeric input (and never validated BatchSize), so those tests stayed skipped on SEA. - Add StatementOptionValidator (poll time >= 0, batch size > 0 long, query timeout >= 0), each throwing ArgumentOutOfRangeException with the exact Thrift message text. - StatementExecutionStatement.SetOption delegates to it. - StatementExecutionStatement constructor validates QueryTimeoutSeconds and BatchSize from config. - StatementExecutionConnection.ValidateProperties validates PollTimeMilliseconds via the same helper (AOORE, allows 0 like Thrift). - Remove the three ValidateCanSetOption* SEA skip overrides + PECO-3011 TODOs so the inherited theories now run on both protocols. Adds StatementOptionValidatorTests (25 cases mirroring the base theory InlineData). The shared validators duplicate the Thrift HiveServer2Statement/ApacheUtility logic for now; consolidating them into the hiveserver2 submodule's ApacheUtility is a tracked follow-up. Co-authored-by: Isaac --- .../StatementExecutionConnection.cs | 7 +- .../StatementExecutionStatement.cs | 24 +++-- .../StatementOptionValidator.cs | 64 +++++++++++++ csharp/test/E2E/StatementTests.cs | 24 +---- .../StatementOptionValidatorTests.cs | 94 +++++++++++++++++++ 5 files changed, 182 insertions(+), 31 deletions(-) create mode 100644 csharp/src/StatementExecution/StatementOptionValidator.cs create mode 100644 csharp/test/Unit/StatementExecution/StatementOptionValidatorTests.cs diff --git a/csharp/src/StatementExecution/StatementExecutionConnection.cs b/csharp/src/StatementExecution/StatementExecutionConnection.cs index bd57dd1de..93281ad53 100644 --- a/csharp/src/StatementExecution/StatementExecutionConnection.cs +++ b/csharp/src/StatementExecution/StatementExecutionConnection.cs @@ -339,8 +339,11 @@ private void ValidateProperties() // PECO-3064: adbc.apache.statement.polltime_ms is the single key for SEA polling cadence // (consolidated with the Thrift path). SEA defaults to 1000 ms — HTTP/JSON polls are // heavier than Thrift's 100 ms default — but both protocols share the same property. - _pollingIntervalMs = PropertyHelper.GetPositiveIntPropertyWithValidation( - properties, ApacheParameters.PollTimeMilliseconds, defaultValue: 1000); + // Validated via StatementOptionValidator so an invalid poll time throws + // ArgumentOutOfRangeException (matching Thrift), allowing 0 (no delay) and up. + _pollingIntervalMs = properties.TryGetValue(ApacheParameters.PollTimeMilliseconds, out string? pollTimeValue) + ? StatementOptionValidator.ValidatePollTime(ApacheParameters.PollTimeMilliseconds, pollTimeValue) + : 1000; // Tracing propagation configuration. Base class (TracingConnection) already handles ActivityTrace init. _tracePropagationEnabled = PropertyHelper.GetBooleanPropertyWithValidation(properties, DatabricksParameters.TracePropagationEnabled, true); diff --git a/csharp/src/StatementExecution/StatementExecutionStatement.cs b/csharp/src/StatementExecution/StatementExecutionStatement.cs index 24419936a..6d3313b1b 100644 --- a/csharp/src/StatementExecution/StatementExecutionStatement.cs +++ b/csharp/src/StatementExecution/StatementExecutionStatement.cs @@ -180,8 +180,17 @@ public StatementExecutionStatement( _waitTimeoutSeconds = waitTimeoutSeconds; _pollingIntervalMs = pollingIntervalMs; _properties = properties ?? throw new ArgumentNullException(nameof(properties)); - _queryTimeoutSeconds = PropertyHelper.GetIntPropertyWithValidation( - properties, ApacheParameters.QueryTimeoutSeconds, 0); + + // Validate config-supplied statement options so bad config throws ArgumentOutOfRangeException + // at CreateStatement time, matching the Thrift path. BatchSize is read-only on SEA — validated + // for parity but otherwise unused at the statement level. + _queryTimeoutSeconds = properties.TryGetValue(ApacheParameters.QueryTimeoutSeconds, out string? queryTimeoutValue) + ? StatementOptionValidator.ValidateQueryTimeout(ApacheParameters.QueryTimeoutSeconds, queryTimeoutValue) + : 0; + if (properties.TryGetValue(ApacheParameters.BatchSize, out string? batchSizeValue)) + { + StatementOptionValidator.ValidateBatchSize(ApacheParameters.BatchSize, batchSizeValue); + } _recyclableMemoryStreamManager = recyclableMemoryStreamManager ?? throw new ArgumentNullException(nameof(recyclableMemoryStreamManager)); _lz4BufferPool = lz4BufferPool ?? throw new ArgumentNullException(nameof(lz4BufferPool)); _httpClient = httpClient ?? throw new ArgumentNullException(nameof(httpClient)); @@ -239,19 +248,18 @@ public override void SetOption(string key, string value) // Validate these options to match Thrift path behavior (throw ArgumentOutOfRangeException // for invalid values), even though they are read-only in SEA (set at connection level). + // The same validators run at construction (see constructor / connection), so SetOption + // and bad-config behave identically to Thrift. case ApacheParameters.PollTimeMilliseconds: - if (string.IsNullOrEmpty(value) || !int.TryParse(value, out int pollTimeMs) || pollTimeMs < 0) - throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value greater than or equal to 0."); + StatementOptionValidator.ValidatePollTime(key, value); break; case ApacheParameters.BatchSize: - if (string.IsNullOrEmpty(value) || !long.TryParse(value, out long batchSize) || batchSize <= 0) - throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value greater than zero."); + StatementOptionValidator.ValidateBatchSize(key, value); break; case ApacheParameters.BatchSizeStopCondition: break; case ApacheParameters.QueryTimeoutSeconds: - if (string.IsNullOrEmpty(value) || !int.TryParse(value, out int queryTimeoutSecs) || queryTimeoutSecs < 0) - throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value of 0 (infinite) or greater."); + StatementOptionValidator.ValidateQueryTimeout(key, value); break; case DatabricksParameters.QueryTags: diff --git a/csharp/src/StatementExecution/StatementOptionValidator.cs b/csharp/src/StatementExecution/StatementOptionValidator.cs new file mode 100644 index 000000000..d07961cea --- /dev/null +++ b/csharp/src/StatementExecution/StatementOptionValidator.cs @@ -0,0 +1,64 @@ +/* +* Copyright (c) 2025 ADBC Drivers Contributors +* +* 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. +*/ + +using System; + +namespace AdbcDrivers.Databricks.StatementExecution +{ + /// + /// Validates the numeric statement options shared with the Thrift path + /// (poll time, batch size, query timeout). Each method throws + /// for any invalid value — non-numeric, + /// out of range, or overflowing — with the exact message text the Thrift driver uses + /// (HiveServer2Statement / ApacheUtility), so SEA and Thrift behave identically. + /// + /// On SEA these options are read-only (set at connection level); the validators exist + /// purely for API/behavior parity — they validate but the value is otherwise unused at + /// the statement level. + /// + internal static class StatementOptionValidator + { + /// Poll-time in milliseconds: a numeric value greater than or equal to 0. + public static int ValidatePollTime(string key, string value) + { + if (string.IsNullOrEmpty(value) || !int.TryParse(value, out int pollTimeMs) || pollTimeMs < 0) + { + throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value greater than or equal to 0."); + } + return pollTimeMs; + } + + /// Batch size: a numeric (long) value greater than zero. + public static long ValidateBatchSize(string key, string value) + { + if (string.IsNullOrEmpty(value) || !long.TryParse(value, out long batchSize) || batchSize <= 0) + { + throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value greater than zero."); + } + return batchSize; + } + + /// Query timeout in seconds: a numeric value of 0 (infinite) or greater. + public static int ValidateQueryTimeout(string key, string value) + { + if (string.IsNullOrEmpty(value) || !int.TryParse(value, out int queryTimeoutSecs) || queryTimeoutSecs < 0) + { + throw new ArgumentOutOfRangeException(key, value, $"The value '{value}' for option '{key}' is invalid. Must be a numeric value of 0 (infinite) or greater."); + } + return queryTimeoutSecs; + } + } +} diff --git a/csharp/test/E2E/StatementTests.cs b/csharp/test/E2E/StatementTests.cs index a2b56cb46..0495b3181 100644 --- a/csharp/test/E2E/StatementTests.cs +++ b/csharp/test/E2E/StatementTests.cs @@ -37,7 +37,6 @@ namespace AdbcDrivers.Databricks.Tests { - // TODO: PECO-3011 - CanSetOptionBatchSize/PollTime/QueryTimeout inherited from base class not validated in SEA's StatementExecutionStatement public class StatementTests : StatementTests { public StatementTests(ITestOutputHelper? outputHelper) @@ -45,26 +44,9 @@ public StatementTests(ITestOutputHelper? outputHelper) { } - // TODO: PECO-3011 - SEA StatementExecutionStatement does not validate poll time option - protected override void ValidateCanSetOptionPollTime(string value, bool throws = false) - { - Skip.If(TestConfiguration.Protocol == "rest", "SEA does not enforce poll time validation"); - base.ValidateCanSetOptionPollTime(value, throws); - } - - // TODO: PECO-3011 - SEA StatementExecutionStatement does not validate query timeout option - protected override void ValidateCanSetOptionQueryTimeout(string value, bool throws = false) - { - Skip.If(TestConfiguration.Protocol == "rest", "SEA does not enforce query timeout validation"); - base.ValidateCanSetOptionQueryTimeout(value, throws); - } - - // TODO: PECO-3011 - SEA StatementExecutionStatement does not validate batch size option - protected override void ValidateCanSetOptionBatchSize(string value, bool throws = false) - { - Skip.If(TestConfiguration.Protocol == "rest", "SEA does not enforce batch size validation"); - base.ValidateCanSetOptionBatchSize(value, throws); - } + // PECO-3011: SEA now validates PollTime/QueryTimeout/BatchSize at both construction and + // SetOption (StatementOptionValidator), matching the Thrift path — so the inherited + // CanSetOption* theories run unmodified on both protocols (no SEA skip override needed). [SkippableTheory] [InlineData(true, "CloudFetch enabled")] diff --git a/csharp/test/Unit/StatementExecution/StatementOptionValidatorTests.cs b/csharp/test/Unit/StatementExecution/StatementOptionValidatorTests.cs new file mode 100644 index 000000000..0d732bdc1 --- /dev/null +++ b/csharp/test/Unit/StatementExecution/StatementOptionValidatorTests.cs @@ -0,0 +1,94 @@ +/* +* Copyright (c) 2025 ADBC Drivers Contributors +* +* 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. +*/ + +using System; +using AdbcDrivers.Databricks.StatementExecution; +using Xunit; + +namespace AdbcDrivers.Databricks.Tests.Unit.StatementExecution +{ + /// + /// Pins the SEA statement-option validation to match the Thrift path (PECO-3011): every invalid + /// value throws . The InlineData mirrors the inherited + /// CanSetOption{PollTime,BatchSize,QueryTimeout} theories so the local check tracks the E2E ones. + /// + public class StatementOptionValidatorTests + { + // --- PollTime: numeric >= 0 --- + [Theory] + [InlineData("-1")] + [InlineData("zero")] + [InlineData("-2147483648")] + [InlineData("2147483648")] // overflows int + [InlineData("")] + public void ValidatePollTime_Invalid_Throws(string value) => + Assert.Throws(() => StatementOptionValidator.ValidatePollTime("k", value)); + + [Theory] + [InlineData("0", 0)] + [InlineData("1", 1)] + [InlineData("2147483647", 2147483647)] + public void ValidatePollTime_Valid_ReturnsValue(string value, int expected) => + Assert.Equal(expected, StatementOptionValidator.ValidatePollTime("k", value)); + + // --- BatchSize: numeric (long) > 0 --- + [Theory] + [InlineData("-1")] + [InlineData("one")] + [InlineData("-2147483648")] + [InlineData("9223372036854775808")] // overflows long + [InlineData("0")] + [InlineData("")] + public void ValidateBatchSize_Invalid_Throws(string value) => + Assert.Throws(() => StatementOptionValidator.ValidateBatchSize("k", value)); + + [Theory] + [InlineData("1", 1L)] + [InlineData("2147483648", 2147483648L)] + [InlineData("9223372036854775807", 9223372036854775807L)] + public void ValidateBatchSize_Valid_ReturnsValue(string value, long expected) => + Assert.Equal(expected, StatementOptionValidator.ValidateBatchSize("k", value)); + + // --- QueryTimeout: numeric >= 0 (0 = infinite) --- + [Theory] + [InlineData("-1")] + [InlineData("zero")] + [InlineData("-2147483648")] + [InlineData("2147483648")] + [InlineData("")] + public void ValidateQueryTimeout_Invalid_Throws(string value) => + Assert.Throws(() => StatementOptionValidator.ValidateQueryTimeout("k", value)); + + [Theory] + [InlineData("0", 0)] + [InlineData("60", 60)] + public void ValidateQueryTimeout_Valid_ReturnsValue(string value, int expected) => + Assert.Equal(expected, StatementOptionValidator.ValidateQueryTimeout("k", value)); + + [Fact] + public void Messages_MatchThriftWording() + { + var poll = Assert.Throws(() => StatementOptionValidator.ValidatePollTime("k", "-1")); + Assert.Contains("Must be a numeric value greater than or equal to 0.", poll.Message); + + var batch = Assert.Throws(() => StatementOptionValidator.ValidateBatchSize("k", "0")); + Assert.Contains("Must be a numeric value greater than zero.", batch.Message); + + var timeout = Assert.Throws(() => StatementOptionValidator.ValidateQueryTimeout("k", "-1")); + Assert.Contains("Must be a numeric value of 0 (infinite) or greater.", timeout.Message); + } + } +}