SNOW-2866776: add private var in numeric type

[sfc-gh-yuwang]

1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

   Fixes SNOW-2866776

2. Fill out the following pre-review checklist:

   • I am adding a new automated test(s) to verify correctness of my new code
     • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
   • I am adding new logging messages
   • I am adding a new telemetry message
   • I am adding new credentials
   • I am adding a new dependency
   • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
   • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines
   • If adding any arguments to public Snowpark APIs or creating new public Snowpark APIs, I acknowledge that I have ensured my changes include AST support. Follow the link for more information: AST Support Guidelines

3. Please describe how your code solves the related issue.

https://snowpark-python-001.jenkinsdev1.us-west-2.aws-dev.app.snowflake.com/job/SnowparkConnectRegressionRunner/220/
scos test

[sfc-gh-joshi]

Suggested change

	def __init__(self, **kwargs) -> None:
	self._precision = kwargs.get("precision", None)
	self._scale = kwargs.get("scale", None)
	def __init__(self, precision: int | None = None, scale: int | None = None) -> None:
	self._precision = precision
	self._scale = scale

Can we do this style instead to make it more explicit?

[sfc-gh-yuwang]

this is a internal only feature, I think we don't want to expose this information to customer?

[sfc-gh-joshi]

OK, we can just keep it as kwargs then.

What do you think about moving the arguments onto _IntegralType instead of _NumericType? It looks like Snowflake only has a single precision for FLOAT and DECIMAL (docs), so we should never have to expose precision information for those types.

[sfc-gh-aling]

+1 on using _IntegralType, I don't think we need _scale as it would always be 0

[sfc-gh-yuwang]

i see, will fix this

[sfc-gh-yuwang]

I modified eq function to exclude _precision and _scale

[sfc-gh-aling]

why we overwrite the logic to exclude the new prop?

[sfc-gh-yuwang]

here is a example test:
expected_fields = [
StructField("COL1", LongType(), nullable=True),
StructField("COL2", LongType(), nullable=True),
]
assert df.schema.fields == expected_fields
we exclude the private var so that these existing test does not fail

[sfc-gh-joshi]

Why do we need a custom hash implementation for this class?

[sfc-gh-yuwang]

TL:DR: this is to fix some test failure
when you defined eq, you need to redefine hash otherwise it is considered None

[sfc-gh-aling]

I think whether excluding precision or not should be based upon the context._is_snowpark_connect_mode.

scos they might want to distinguish LongType by precision

[graphite-app]

Grammar Error in Exception Message

The error message uses singular "argument" but will be grammatically incorrect when multiple arguments are passed.

# Current: "takes 0 argument but 2 were given" (incorrect grammar)

Fix: Use proper singular/plural form:

arg_word = "argument" if len(kwargs) == 1 else "arguments"
raise TypeError(
    f"__init__() takes 0 arguments but {len(kwargs)} {arg_word if len(kwargs) != 1 else 'was'} given"
)

Or simply use plural consistently:

raise TypeError(
    f"__init__() takes 0 arguments but {len(kwargs)} were given"
)

Spotted by Graphite Agent



Is this helpful? React 👍 or 👎 to let us know.

[sfc-gh-yuwang]

I still kept the eq function in TimedeltaType class.
Otherwise it will treat LongType() == TimedeltaType() which is wrong.