Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
//
//*********************************************************************
using System;
using System.Data.SqlTypes;
using Microsoft.Data.Analysis;
using static Microsoft.SqlServer.CSharpExtension.Sql;
using static Microsoft.SqlServer.CSharpExtension.SqlNumericHelper;

namespace Microsoft.SqlServer.CSharpExtension
{
Expand Down Expand Up @@ -126,6 +128,9 @@ private unsafe void AddColumn(
case SqlDataType.DotNetReal:
AddDataFrameColumn<float>(columnNumber, rowsNumber, colData, colMap);
break;
case SqlDataType.DotNetNumeric:
AddNumericDataFrameColumn(columnNumber, rowsNumber, colData, colMap);
break;
case SqlDataType.DotNetChar:
int[] strLens = new int[rowsNumber];
Interop.Copy((int*)colMap, strLens, 0, (int)rowsNumber);
Expand Down Expand Up @@ -185,5 +190,54 @@ private unsafe void AddDataFrameColumn<T>(

CSharpDataFrame.Columns.Add(colDataFrame);
}

/// <summary>
/// This method adds NUMERIC/DECIMAL column data by converting from SQL_NUMERIC_STRUCT
/// to SqlDecimal values (full 38-digit precision), creating a PrimitiveDataFrameColumn<SqlDecimal>,
/// and adding it to the DataFrame.
/// </summary>
/// <param name="columnNumber">The column index.</param>
/// <param name="rowsNumber">Number of rows in this column.</param>
/// <param name="colData">Pointer to array of SQL_NUMERIC_STRUCT structures.</param>
/// <param name="colMap">Pointer to null indicator array (SQL_NULL_DATA for null values).</param>
private unsafe void AddNumericDataFrameColumn(
ushort columnNumber,
ulong rowsNumber,
void *colData,
int *colMap)
{
// Cast the raw pointer to SQL_NUMERIC_STRUCT array
//
SqlNumericStruct* numericArray = (SqlNumericStruct*)colData;

// Create a DataFrame column for SqlDecimal values
// Using SqlDecimal instead of decimal provides full SQL Server precision (38 digits)
//
PrimitiveDataFrameColumn<SqlDecimal> colDataFrame =
new PrimitiveDataFrameColumn<SqlDecimal>(_columns[columnNumber].Name, (int)rowsNumber);

// Convert each SQL_NUMERIC_STRUCT to SqlDecimal, handling nulls
//
Span<int> nullSpan = new Span<int>(colMap, (int)rowsNumber);
for (int i = 0; i < (int)rowsNumber; ++i)
{
// Check if this row has a null value
//
// Why check both Nullable == 0 and SQL_NULL_DATA?
// - Nullable == 0 means column is declared NOT NULL (cannot contain nulls)
// - For NOT NULL columns, skip null checking for performance (nullSpan[i] is undefined)
// - For nullable columns (Nullable != 0), check if nullSpan[i] == SQL_NULL_DATA (-1)
// - This matches the pattern used by other numeric types in the codebase
if (_columns[columnNumber].Nullable == 0 || nullSpan[i] != SQL_NULL_DATA)
{
// Convert SQL_NUMERIC_STRUCT to SqlDecimal with full precision support
//
colDataFrame[i] = ToSqlDecimal(numericArray[i]);
}
// else: leave as null (default for nullable primitive column)
}

CSharpDataFrame.Columns.Add(colDataFrame);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@
//
//*********************************************************************
using System;
using System.Data.SqlTypes;
using System.Linq;
using System.Text;
using System.Runtime.InteropServices;
using System.Collections.Generic;
using Microsoft.Data.Analysis;
using static Microsoft.SqlServer.CSharpExtension.Sql;
using static Microsoft.SqlServer.CSharpExtension.SqlNumericHelper;

namespace Microsoft.SqlServer.CSharpExtension
{
Expand Down Expand Up @@ -174,6 +176,9 @@ DataFrameColumn column
case SqlDataType.DotNetDouble:
SetDataPtrs<double>(columnNumber, GetArray<double>(column));
break;
case SqlDataType.DotNetNumeric:
ExtractNumericColumn(columnNumber, column);
break;
case SqlDataType.DotNetChar:
// Calculate column size from actual data.
// columnSize = max UTF-8 byte length across all rows.
Expand Down Expand Up @@ -203,7 +208,7 @@ DataFrameColumn column
/// <summary>
/// This method sets data pointer for the column and append the array to the handle list.
/// </summary>
private unsafe void SetDataPtrs<T>(
private void SetDataPtrs<T>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from private unsafe void SetDataPtrs<T> to private void SetDataPtrs<T> is technically correct (the method body uses GCHandle which doesn't need unsafe), but it's unrelated to the DECIMAL feature. If it's intentional cleanup, call it out in the PR description. Otherwise, revert to keep the diff focused.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from private unsafe void SetDataPtrs<T> to private void SetDataPtrs<T> is technically correct (the method body uses GCHandle which doesn't need unsafe), but it's unrelated to the DECIMAL feature. If it's intentional cleanup, call it out in the PR description. Otherwise, revert to keep the diff focused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's intentional; I've been trying to get rid of unsafe as much as possible.

I may revert it though to keep the change focused.

ushort columnNumber,
T[] array
) where T : unmanaged
Expand All @@ -213,6 +218,68 @@ T[] array
_handleList.Add(handle);
}

/// <summary>
/// Extracts NUMERIC/DECIMAL column data by converting SqlDecimal values to ODBC-compatible SQL_NUMERIC_STRUCT array.
/// </summary>
/// <param name="columnNumber">The column index.</param>
/// <param name="column">The DataFrameColumn containing SqlDecimal values.</param>
private unsafe void ExtractNumericColumn(
ushort columnNumber,
DataFrameColumn column)
{
if (column == null)
{
SetDataPtrs<SqlNumericStruct>(columnNumber, Array.Empty<SqlNumericStruct>());
return;
}

// Determine target precision/scale from max values across all rows
//
byte precision = SqlNumericHelper.SQL_MIN_PRECISION;
byte scale = (byte)_columns[columnNumber].DecimalDigits;

for (int rowNumber = 0; rowNumber < column.Length; ++rowNumber)
{
if (column[rowNumber] != null)
{
SqlDecimal value = (SqlDecimal)column[rowNumber];
if (!value.IsNull)
{
scale = Math.Max(scale, value.Scale);
precision = Math.Max(precision, value.Precision);
}
}
}

// Enforce T-SQL DECIMAL(p,s) constraints: 1 <= p <= 38, 0 <= s <= p
//
precision = Math.Max(precision, SqlNumericHelper.SQL_MIN_PRECISION);
precision = Math.Min(precision, SqlNumericHelper.SQL_MAX_PRECISION);
if (scale > precision)
{
precision = scale;
}
Comment on lines +236 to +261
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExtractNumericColumn derives target precision as max(value.Precision) and target scale as max(value.Scale) independently. If a column contains values with different scales, increasing the chosen scale can require a larger precision for values with large integer parts (e.g., 123 + scale 2 => 123.00 needs precision 5), which will make FromSqlDecimal throw on conversion. Consider computing targetPrecision as max((value.Precision - value.Scale) + targetScale) across non-null values (where targetScale is the chosen max scale), so all values remain representable after scale normalization.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 — this is a real bug, not just a nit. The independent max(precision) / max(scale) logic will throw OverflowException at runtime for any column with mixed scales.

Concrete scenario: value 123 (p=3, s=0) and 0.99 (p=2, s=2) in the same column → chosen scale=2, chosen precision=3 → FromSqlDecimal tries to produce 123.00 which needs precision 5 inside a DECIMAL(3,2).

This will hit production any time a user's C# executor returns a SqlDecimal column where rows have different intrinsic scales. Please fix before merge.


// Update metadata: Size = precision (total digits), DecimalDigits = scale (fractional digits)
//
_columns[columnNumber].Size = precision;
_columns[columnNumber].DecimalDigits = scale;

Logging.Trace($"ExtractNumericColumn: Column {columnNumber}, T-SQL type=DECIMAL({precision},{scale}), RowCount={column.Length}");

// Convert all values (including NULLs) to SQL_NUMERIC_STRUCT using FromSqlDecimal
//
SqlNumericStruct[] numericArray = new SqlNumericStruct[column.Length];
for (int rowNumber = 0; rowNumber < column.Length; ++rowNumber)
{
SqlDecimal value = (column[rowNumber] != null) ? (SqlDecimal)column[rowNumber] : SqlDecimal.Null;
numericArray[rowNumber] = FromSqlDecimal(value, precision, scale);
Logging.Trace($"ExtractNumericColumn: Row {rowNumber}, Value={value}");
}

SetDataPtrs<SqlNumericStruct>(columnNumber, numericArray);
}

/// <summary>
/// This method gets the array from a DataFrameColumn Column for numeric types.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@
//
//*************************************************************************************************
using System;
using System.Data.SqlTypes;
using System.Runtime;
using System.Text;
using System.Collections.Generic;
using System.Runtime.InteropServices;
using static Microsoft.SqlServer.CSharpExtension.Sql;
using static Microsoft.SqlServer.CSharpExtension.SqlNumericHelper;

namespace Microsoft.SqlServer.CSharpExtension
{
Expand Down Expand Up @@ -132,6 +134,11 @@ public unsafe void AddParam(
case SqlDataType.DotNetBit:
_params[paramNumber].Value = *(bool*)paramValue;
break;
case SqlDataType.DotNetNumeric:
// Convert SQL_NUMERIC_STRUCT to SqlDecimal, handling OUTPUT parameter sentinel (precision=0)
//
_params[paramNumber].Value = ToSqlDecimalFromPointer((SqlNumericStruct*)paramValue);
break;
case SqlDataType.DotNetChar:
_params[paramNumber].Value = Interop.UTF8PtrToStr((char*)paramValue, (ulong)strLenOrNullMap);
break;
Expand Down Expand Up @@ -168,7 +175,19 @@ public unsafe void ReplaceParam(

_params[paramNumber].Value = paramValue_;
CSharpParam param = _params[paramNumber];
if(param.Value == null)

// Use null-coalescing pattern for safer null checking with value types
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "Use null-coalescing pattern for safer null checking with value types" but this isn't a null-coalescing pattern — it's a split null check.

The split is needed because SqlDecimal is a value type that gets boxed when stored in the object-typed param.Value. For SqlDecimal.Null, the box itself is non-null (it's a valid object reference to a boxed struct), so the original == null would have returned false. ReferenceEquals catches true null, and the second check catches SqlDecimal.Null.

Please update the comment to explain this specific reason rather than using inaccurate terminology. Something like:
"SqlDecimal is a value type — when boxed, SqlDecimal.Null is a non-null object reference, so we check ReferenceEquals first for true null, then SqlDecimal.IsNull for the struct-level null sentinel."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "Use null-coalescing pattern for safer null checking with value types" but this isn't a null-coalescing pattern — it's a split null check.

The split is needed because SqlDecimal is a value type that gets boxed when stored in the object-typed param.Value. For SqlDecimal.Null, the box itself is non-null (it's a valid object reference to a boxed struct), so the original == null would have returned false. ReferenceEquals catches true null, and the second check catches SqlDecimal.Null.

Please update the comment to explain this specific reason rather than using inaccurate terminology. Something like:
"SqlDecimal is a value type — when boxed, SqlDecimal.Null is a non-null object reference, so we check ReferenceEquals first for true null, then SqlDecimal.IsNull for the struct-level null sentinel."

// SqlDecimal is a struct, so we need to check both object null and SqlDecimal.IsNull
//
if(ReferenceEquals(param.Value, null))
{
*strLenOrNullMap = SQL_NULL_DATA;
return;
}

// Special handling for SqlDecimal.Null (SqlDecimal is a struct, not a class)
//
if(param.DataType == SqlDataType.DotNetNumeric && param.Value is SqlDecimal sqlDecVal && sqlDecVal.IsNull)
{
*strLenOrNullMap = SQL_NULL_DATA;
return;
Expand Down Expand Up @@ -214,6 +233,21 @@ public unsafe void ReplaceParam(
bool boolValue = Convert.ToBoolean(param.Value);
ReplaceNumericParam<bool>(boolValue, paramValue);
break;
case SqlDataType.DotNetNumeric:
// Use declared precision/scale from T-SQL parameter definition, not SqlDecimal's intrinsic values
//
if (param.Value is SqlDecimal sqlDecimalValue)
{
byte precision = (byte)param.Size;
byte scale = (byte)param.DecimalDigits;
*strLenOrNullMap = Sql.SqlNumericStructSize;
ReplaceNumericStructParam(sqlDecimalValue, precision, scale, paramValue);
}
else
{
throw new InvalidCastException($"Expected SqlDecimal for NUMERIC parameter, got {param.Value?.GetType().Name ?? "null"}");
}
break;
case SqlDataType.DotNetChar:
// For CHAR/VARCHAR, strLenOrNullMap is in bytes (1 byte per character for ANSI).
// param.Size is the declared parameter size in characters (from SQL Server's CHAR(n)/VARCHAR(n)).
Expand All @@ -229,7 +263,6 @@ public unsafe void ReplaceParam(
// In C#, sizeof(char) is always 2 bytes (UTF-16), regardless of platform.
// Note: C++ wchar_t is 2 bytes on Windows but 4 bytes on Linux - this extension only supports Windows.
// param.Size is the declared parameter size in characters (from SQL Server's NCHAR(n)/NVARCHAR(n)),
// so we multiply by sizeof(char) to convert to bytes.
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line // so we multiply by sizeof(char) to convert to bytes. was deleted from this NCHAR/NVARCHAR comment block. This appears to be an accidental deletion — the remaining comment references param.Size being "the declared parameter size in characters" but no longer explains the multiplication on the next line. Please restore it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line // so we multiply by sizeof(char) to convert to bytes. was deleted from this NCHAR/NVARCHAR comment block. This appears to be an accidental deletion — the remaining comment references param.Size being "the declared parameter size in characters" but no longer explains the multiplication on the next line. Please restore it.

int wcharByteLen = param.Value.Length * sizeof(char);
int wcharMaxByteLen = (int)param.Size * sizeof(char);
Expand Down Expand Up @@ -275,6 +308,26 @@ private unsafe void ReplaceNumericParam<T>(
*paramValue = (void*)handle.AddrOfPinnedObject();
}

/// <summary>
/// Replaces NUMERIC/DECIMAL parameter value by converting SqlDecimal to SQL_NUMERIC_STRUCT and pinning it.
/// </summary>
private unsafe void ReplaceNumericStructParam(
SqlDecimal value,
byte precision,
byte scale,
void **paramValue)
{
SqlNumericStruct numericStruct = FromSqlDecimal(value, precision, scale);

// Box into array for heap allocation (stack-allocated struct destroyed at method exit)
// Pin to prevent GC from moving memory while native code holds the pointer
//
SqlNumericStruct[] valueArray = new SqlNumericStruct[1] { numericStruct };
GCHandle handle = GCHandle.Alloc(valueArray, GCHandleType.Pinned);
_handleList.Add(handle);
*paramValue = (void*)handle.AddrOfPinnedObject();
}

/// <summary>
/// This method replaces parameter value for string data types.
/// If the string is not empty, the address of underlying bytes will be assigned to paramValue.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
<EnableDynamicLoading>true</EnableDynamicLoading>
</PropertyGroup>
<PropertyGroup>
<BinRoot Condition=" '$(BinRoot)' == '' ">$(MSBuildThisFileDirectory)..\..\..\..\build-output\dotnet-core-CSharp-extension\windows</BinRoot>
<OutputPath>$(BinRoot)/$(Configuration)/</OutputPath>
<AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath>
<RollForward>LatestMajor</RollForward>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Data.Analysis" Version="0.23.0"/>
<PackageReference Include="Microsoft.Data.SqlClient" Version="5.2.2"/>
</ItemGroup>
</Project>
21 changes: 17 additions & 4 deletions language-extensions/dotnet-core-CSharp/src/managed/utils/Sql.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@
// @File: Sql.cs
//
// Purpose:
// This is the the main include for SqlDataType and Sql return values
// SQL data type definitions, ODBC constants, and type mapping dictionaries.
// For NUMERIC/DECIMAL conversion utilities, see SqlNumericHelper.cs.
//
//*********************************************************************
using System;
using System.Collections.Generic;
using System.Text;
using System.Data.SqlTypes;
using System.Runtime.InteropServices;

namespace Microsoft.SqlServer.CSharpExtension
{
Expand All @@ -27,6 +29,15 @@ public class Sql

public const short MinUtf8CharSize = 1;
public const short MinUtf16CharSize = 2;

/// <summary>
/// Size of SQL_NUMERIC_STRUCT in bytes (ODBC specification).
/// Dynamically calculated from SqlNumericHelper.SqlNumericStruct layout:
/// precision(1) + scale(1) + sign(1) + val[16] = 19 bytes.
/// Must match the exact size of ODBC's SQL_NUMERIC_STRUCT for binary compatibility.
/// </summary>
public static readonly short SqlNumericStructSize = (short)Marshal.SizeOf<SqlNumericHelper.SqlNumericStruct>();

public enum SqlDataType: short
{
DotNetBigInt = -5 + SQL_SIGNED_OFFSET, //SQL_C_SBIGINT + SQL_SIGNED_OFFSET
Expand Down Expand Up @@ -68,7 +79,8 @@ public enum SqlDataType: short
{typeof(float), SqlDataType.DotNetReal},
{typeof(double), SqlDataType.DotNetDouble},
{typeof(bool), SqlDataType.DotNetBit},
{typeof(string), SqlDataType.DotNetChar}
{typeof(string), SqlDataType.DotNetChar},
{typeof(SqlDecimal), SqlDataType.DotNetNumeric}
};

/// <summary>
Expand All @@ -89,7 +101,8 @@ public enum SqlDataType: short
{SqlDataType.DotNetDouble, sizeof(double)},
{SqlDataType.DotNetBit, sizeof(bool)},
{SqlDataType.DotNetChar, MinUtf8CharSize},
{SqlDataType.DotNetWChar, MinUtf16CharSize}
{SqlDataType.DotNetWChar, MinUtf16CharSize},
{SqlDataType.DotNetNumeric, SqlNumericStructSize}
};

/// <summary>
Expand Down
Loading
Loading