Skip to content

fix(csharp): treat catalog as a literal name on the Thrift metadata path (#525) #574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eric-wang-1990 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(csharp): treat catalog as a literal name on the Thrift metadata path (#525) #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
47 changes: 47 additions & 0 deletions csharp/src/DatabricksStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,35 @@ private void HandleSparkCatalog()
CatalogName = DatabricksConnection.HandleSparkCatalog(CatalogName);
}

/// <summary>
/// Normalizes the catalog argument for the Thrift metadata path (GetSchemas/GetTables/GetColumns)
/// so that catalog behaves as an exact-name identifier rather than a SQL-LIKE pattern (issue #525),
/// matching the SEA path and PR #536.
///
/// - A bare match-all wildcard ("%" or "*") — like a null argument — means "all catalogs", so it is
/// mapped to null. On Thrift the server already expands "%"/"*", but mapping to null keeps the
/// behavior identical to the SEA path (StatementExecutionStatement.EffectiveCatalog).
/// - Any other catalog name has its "_" and "%" wildcard characters escaped so the Thrift server
/// matches it literally. The server honors the "\_"/"\%" escape (verified against a live warehouse);
/// without escaping it treats "_" as a single-char wildcard, so e.g. catalog "comparator_tests"
/// would also match "comparator-tests".
///
/// When <paramref name="escapePatternWildcards"/> is true the base class (HiveServer2Statement) already
/// escapes the catalog argument, so the name is returned unescaped here (after the bare-wildcard
/// normalization) to avoid double-escaping.
/// </summary>
internal static string? NormalizeCatalogForThrift(string? catalog, bool escapePatternWildcards)
{
if (catalog == null || catalog == "%" || catalog == "*")
{
return null;
}

return escapePatternWildcards
? catalog
: catalog.Replace("_", "\\_").Replace("%", "\\%");
}

/// <summary>
/// Helper method that returns the fully qualified table name enclosed by backtick.
/// The returned value can be used as table name in the SQL statement
Expand Down Expand Up @@ -764,6 +793,12 @@ protected override async Task<QueryResult> GetSchemasAsync(CancellationToken can
HandleSparkCatalog();
activity?.SetTag("statement.catalog_name_after_spark_handling", CatalogName ?? "(none)");

// Issue #525: on Thrift, catalog is an exact-name identifier, not a LIKE pattern.
// Map a bare match-all wildcard to null ("all catalogs") and escape any other catalog
// name so the server matches it literally (parity with the SEA path / PR #536).
CatalogName = NormalizeCatalogForThrift(CatalogName, EscapePatternWildcards);
activity?.SetTag("statement.catalog_name_after_pattern_normalization", CatalogName ?? "(none)");

// If EnableMultipleCatalogSupport is false and catalog is not null or SPARK, return empty result without RPC call
if (!enableMultipleCatalogSupport && CatalogName != null)
{
Expand Down Expand Up @@ -810,6 +845,12 @@ protected override async Task<QueryResult> GetTablesAsync(CancellationToken canc
HandleSparkCatalog();
activity?.SetTag("statement.catalog_name_after_spark_handling", CatalogName ?? "(none)");

// Issue #525: on Thrift, catalog is an exact-name identifier, not a LIKE pattern.
// Map a bare match-all wildcard to null ("all catalogs") and escape any other catalog
// name so the server matches it literally (parity with the SEA path / PR #536).
CatalogName = NormalizeCatalogForThrift(CatalogName, EscapePatternWildcards);
activity?.SetTag("statement.catalog_name_after_pattern_normalization", CatalogName ?? "(none)");

// If EnableMultipleCatalogSupport is false and catalog is not null or SPARK, return empty result without RPC call
if (!enableMultipleCatalogSupport && CatalogName != null)
{
Expand Down Expand Up @@ -856,6 +897,12 @@ protected override async Task<QueryResult> GetColumnsAsync(CancellationToken can
HandleSparkCatalog();
activity?.SetTag("statement.catalog_name_after_spark_handling", CatalogName ?? "(none)");

// Issue #525: on Thrift, catalog is an exact-name identifier, not a LIKE pattern.
// Map a bare match-all wildcard to null ("all catalogs") and escape any other catalog
// name so the server matches it literally (parity with the SEA path / PR #536).
CatalogName = NormalizeCatalogForThrift(CatalogName, EscapePatternWildcards);
activity?.SetTag("statement.catalog_name_after_pattern_normalization", CatalogName ?? "(none)");

// If EnableMultipleCatalogSupport is false and catalog is not null, return empty result without RPC call
if (!enableMultipleCatalogSupport && CatalogName != null)
{
Expand Down
31 changes: 31 additions & 0 deletions csharp/test/Unit/DatabricksStatementUnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,37 @@ private DatabricksStatement CreateStatement()
return (Dictionary<string, string>?)field?.GetValue(statement);
}

/// <summary>
/// Issue #525: on the Thrift metadata path the catalog argument must behave as an exact-name
/// identifier, not a SQL-LIKE pattern. NormalizeCatalogForThrift maps a bare match-all wildcard
/// ("%"/"*") — like null — to null ("all catalogs"), and escapes "_"/"%" in any other catalog
/// name so the server matches it literally. When EscapePatternWildcards is already enabled the
/// base class escapes catalog itself, so the literal name is returned unchanged to avoid
/// double-escaping.
/// </summary>
[Theory]
// bare match-all wildcards (and null) -> null ("all catalogs"), regardless of the escape flag
[InlineData(null, false, null)]
[InlineData("%", false, null)]
[InlineData("*", false, null)]
[InlineData("%", true, null)]
[InlineData("*", true, null)]
// plain name with no wildcard chars -> unchanged
[InlineData("main", false, "main")]
[InlineData("main", true, "main")]
// name containing "_" -> escaped so it matches literally (flag off)
[InlineData("comparator_tests", false, "comparator\\_tests")]
[InlineData("hive_metastore", false, "hive\\_metastore")]
// "%" inside a name -> escaped too (flag off)
[InlineData("a%b_c", false, "a\\%b\\_c")]
// flag on: base class escapes, so leave the literal name unchanged here (no double-escape)
[InlineData("comparator_tests", true, "comparator_tests")]
public void NormalizeCatalogForThrift_NormalizesAndEscapesCatalog(string? input, bool escapePatternWildcards, string? expected)
{
string? actual = DatabricksStatement.NormalizeCatalogForThrift(input, escapePatternWildcards);
Assert.Equal(expected, actual);
}

/// <summary>
/// Tests that query_tags parameter is captured and added to confOverlay.
/// </summary>
Expand Down
Loading