diff --git a/csharp/src/StatementExecution/StatementExecutionConnection.cs b/csharp/src/StatementExecution/StatementExecutionConnection.cs index bd57dd1d..93281ad5 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 b910097a..6d3313b1 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)); @@ -237,12 +246,20 @@ 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). + // The same validators run at construction (see constructor / connection), so SetOption + // and bad-config behave identically to Thrift. case ApacheParameters.PollTimeMilliseconds: + StatementOptionValidator.ValidatePollTime(key, value); + break; case ApacheParameters.BatchSize: + StatementOptionValidator.ValidateBatchSize(key, value); + break; case ApacheParameters.BatchSizeStopCondition: + break; case ApacheParameters.QueryTimeoutSeconds: + 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 00000000..d07961ce --- /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 a2b56cb4..0495b318 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 00000000..0d732bdc --- /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); + } + } +}