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

feat: Basic functions implementation #3269

Merged
merged 45 commits into from
Dec 12, 2024

Conversation

sfc-gh-asawicki
Copy link
Collaborator

@sfc-gh-asawicki sfc-gh-asawicki commented Dec 11, 2024

Prepare most of the java resource implementation:

  • add common drop method (also to procedures)
  • fix parameters schema and add mapping (also for procedures)
  • fix parameter handling
  • handle parameters and test
  • handle arguments, return type, runtime version, imports, target path, language
  • add default values to arguments
  • improve function details with mapping logic (for easier use in resource)
  • add a bunch of common functions to handle this family of resources
  • handle basic rename
  • add TABLE data type (needs logic and tests)
  • handle external language change
  • test no arguments
  • regenerate model builders and the docs
  • rename a few attributes
  • change requirements for some fields

Next PRs:

  • TABLE function improvements and tests
  • handle secrets, external access integrations, packages, return not null, and comments
  • Add a similar PR for java procedure (reuse what we can)
  • Add PR with all other function types
  • datasources

@sfc-gh-asawicki sfc-gh-asawicki marked this pull request as ready for review December 11, 2024 16:55
Copy link

Integration tests failure for 20601b190475aa6378d8db95935130919d84ea39

"target_path": {
Type: schema.TypeString,
Type: schema.TypeSet,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if there may be issues with it, but I think with objects (one element lists) the preferred type would be schema.TypeList (I imagine there may be some annoying case coming form the fact the the item is saved from its hash value and not just at the index 0).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, the tests passed with setting it as set, it won't be a breaking change if we'll decide to change the type later (schema will stay the same for the user), so I will leave it as is and pray :D

pkg/resources/function_commons.go Show resolved Hide resolved
pkg/resources/function_commons.go Outdated Show resolved Hide resolved
pkg/resources/function_java.go Show resolved Hide resolved
pkg/sdk/functions_ext.go Show resolved Hide resolved
return nil, fmt.Errorf("arg %s cannot be split into arg name, data type, and default", arg)
}
argName := trimmed[:idx]
rest := strings.TrimSpace(trimmed[idx:])
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 isn't DEFAULT as part of signature (after data type)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not, check create function for Java - default argument value - it's part of the SHOW (arguments) but not a part of DESCRIBE's signature. Still: it does not provide a value, so the only thing we can do in the future is to use this DEFAULT from SHOW to discover external changes in adding/removing the DEFAULT overall.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Kk I was just wondering what we are parsing here, if the input is coming from describe's signature then it's fine. I was just wondering if strings.TrimSpace(trimmed[idx:]) could possibly encounter something more than just data type.

Copy link

Integration tests failure for afaba5a68d6a21b0db8ea520de1d1a7e90803dd2

@@ -105,6 +112,11 @@ func (c *IdsGenerator) RandomSchemaObjectIdentifierWithArguments(arguments ...sd
return sdk.NewSchemaObjectIdentifierWithArguments(c.SchemaId().DatabaseName(), c.SchemaId().Name(), c.Alpha(), arguments...)
}

func (c *IdsGenerator) RandomSchemaObjectIdentifierWithArgumentsNewDataTypes(arguments ...datatypes.DataType) sdk.SchemaObjectIdentifierWithArguments {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: use RandomSchemaObjectIdentifierWithArgumentsOldDataTypes above instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted

if idx < 0 {
return nil, fmt.Errorf("part %s cannot be split into stage and path", location)
}
stageRaw := strings.TrimPrefix(strings.TrimSpace(location[:idx]), "@")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about stages with / in their name? If they're not supported, let's mention it in the docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we will

@@ -214,6 +230,11 @@ func functionBaseSchema() map[string]schema.Schema {
DiffSuppressFunc: DiffSuppressDataTypes,
Description: "The argument type.",
},
"arg_default_value": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could drop arg_ prefix, in other fields as well, wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I will do it.

@sfc-gh-asawicki sfc-gh-asawicki merged commit 6d4a103 into main Dec 12, 2024
8 of 9 checks passed
@sfc-gh-asawicki sfc-gh-asawicki deleted the basic-functions-implementation branch December 12, 2024 11:06
sfc-gh-asawicki added a commit that referenced this pull request Dec 12, 2024
Prepare most of the java procedure resource implementation (based on
#3269, check it for details); additionally:
- extracted more common functions to reuse between functions and
procedures - left TODO for some that we duplicate for now

Next PRs:
- handle secrets, external access integrations, packages, return not
null, and comments
- TABLE function improvements and tests
- Add PR with all other function types
- datasources
sfc-gh-jcieslak pushed a commit that referenced this pull request Dec 12, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.100.0](v0.99.0...v0.100.0)
(2024-12-12)


### 🎉 **What's new:**

* Account v1 readiness
([#3236](#3236))
([5df33a8](5df33a8))
* Account v1 readiness generated files
([#3242](#3242))
([3df59dd](3df59dd))
* Account v1 readiness resource
([#3252](#3252))
([8f5698d](8f5698d))
* Add a new account roles data source
([#3257](#3257))
([b3d6b9e](b3d6b9e))
* Add account data source
([#3261](#3261))
([6087fc9](6087fc9))
* Add all other functions and procedures implementations
([#3275](#3275))
([7a6f68d](7a6f68d))
* Basic functions implementation
([#3269](#3269))
([6d4a103](6d4a103))
* Basic procedures implementation
([#3271](#3271))
([933335f](933335f))
* Docs, test, and missing parameter
([#3280](#3280))
([10517f3](10517f3))
* Functions and procedures schemas and generated sources
([#3262](#3262))
([9b70f87](9b70f87))
* Functions sdk update
([#3254](#3254))
([fc1eace](fc1eace))
* Handle missing fields in function and procedure
([#3273](#3273))
([53e7a0a](53e7a0a))
* Procedures schemas and generated sources
([#3263](#3263))
([211ad46](211ad46))
* Procedures sdk update
([#3255](#3255))
([682606a](682606a))
* Rework account parameter resource
([#3264](#3264))
([15aa9c2](15aa9c2))
* Rework data types
([#3244](#3244))
([05ada91](05ada91))
* support table data type
([#3274](#3274))
([13401d5](13401d5))
* Tag association v1 readiness
([#3210](#3210))
([04f6d54](04f6d54))
* Test imports and small fixes
([#3276](#3276))
([a712195](a712195))
* Unsafe execute v1 readiness
([#3266](#3266))
([c4f1e8f](c4f1e8f))
* Use new data types in sql builder for functions and procedures
([#3247](#3247))
([69f677a](69f677a))


### 🔧 **Misc**

* Add ShowByID filtering generation
([#3227](#3227))
([548ec42](548ec42))
* Adress small task-related todos
([#3243](#3243))
([40de9ae](40de9ae))
* Apply masking
([#3234](#3234))
([c209a8a](c209a8a))
* fix missing references in toOpts and changes with newlines
([#3240](#3240))
([246547f](246547f))
* function tests
([#3279](#3279))
([5af6efb](5af6efb))
* Improve config builders
([#3207](#3207))
([425787c](425787c))
* Revert to proper env
([#3238](#3238))
([5d4ed3b](5d4ed3b))
* Use service user for ci
([#3228](#3228))
([2fb50d7](2fb50d7))


### 🐛 **Bug fixes:**

* Make blocked_roles_field optional in OAuth security integrations
([#3267](#3267))
([7197b57](7197b57))
* Minor fixes
([#3231](#3231))
([1863bf6](1863bf6))
* Minor fixes 2
([#3230](#3230))
([73b7e74](73b7e74))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
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.

3 participants