Skip to content
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.

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

fix(csharp/src/Drivers/Apache): set the precision and scale correctly on Decimal128Type #1858

Merged

Conversation

birschick-bq
Copy link
Contributor

Addresses a TODO item to accurately retrieve the precision and scale for DECIMAL (Decimal128Type) when retrieving table schema (GetTableSchema).

  • Parses the type name to find the precision and scale, using the default of 10 and 0, respectively.

@birschick-bq birschick-bq marked this pull request as ready for review May 13, 2024 21:31
Copy link
Contributor

@CurtHagenlocher CurtHagenlocher left a comment

Choose a reason for hiding this comment

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

Thanks for the change! I don't understand how decimal types are even usable if we don't know the scale of the result. Are there known circumstances where that's true?

How were 10 and 0 chosen as the defaults for precision and scale?

Group scaleGroup = groups["scale"];

precision = precisionGroup.Success ? int.Parse(precisionGroup.Value) : precision;
scale = scaleGroup.Success ? int.Parse(scaleGroup.Value) : scale;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be TryParse; the capture might succeed but capture so many digits that the Parse will throw an exception. Alternatively, the regex could probably specify a range of lengths -- but TryParse seems like a safer and more readable approach to me.

// TODO: Parse typeName for precision and scale, because not available in other metadata.
return new Decimal128Type(38, 38);
// Note: parsing the type definition is only viable at the table level. Won't
// work for statement results.
Copy link
Contributor

Choose a reason for hiding this comment

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

What will happen for statement results? And does that code path go through here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Statement results has its own metadata. The statement's result set metadata DOES contain precision and scale. So runtime was already working. This is only for the AdbcConnection.GetTableMetadata path.

I think this comment might be misleading. What is meant to convey is that this technique can't be used for handling complex types like ARRAY, MAP, and STRUCT with statement results.

Might just remove the comment.

@lidavidm lidavidm added this to the ADBC Libraries 13 milestone May 14, 2024
@birschick-bq
Copy link
Contributor Author

Thanks for the change! I don't understand how decimal types are even usable if we don't know the scale of the result. Are there known circumstances where that's true?

How were 10 and 0 chosen as the defaults for precision and scale?

These changes affect AdbcConnection.GetTableSchema and not any of the ExecuteQuery/ExecuteUpdate. They (statement results) have a separate metadata that DOES include the precision and scale. (Unfortunately, statement result metadata does not include metadata for complex types.)

The default are chose from https://docs.databricks.com/en/sql/language-manual/data-types/decimal-type.html#syntax

            // { DECIMAL | DEC | NUMERIC } [ (  p [ , s ] ) ]
            // p: Optional maximum precision (total number of digits) of the number between 1 and 38. The default is 10.
            // s: Optional scale of the number between 0 and p. The number of digits to the right of the decimal point. The default is 0.

@CurtHagenlocher CurtHagenlocher merged commit 57770e3 into apache:main May 14, 2024
6 checks passed
@birschick-bq birschick-bq deleted the dev/birschick-bq/get-table-schema branch June 4, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants