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!: EXPOSED-288 Extend ANY and ALL operators to use ArrayColumnType #1992

Merged
merged 6 commits into from
Feb 20, 2024

Conversation

bog-walk
Copy link
Member

  1. Replace UntypedAndUnsizedArrayColumnType from PR feat: Add ALL and ANY operators accepting array, subquery, or table parameters #1886 with ArrayColumnType from PR feat: EXPOSED-248 Support array column type #1986
    • Remove unused dialect data types
  2. Introduce function resolveColumnType() that returns an appropriate column type based on type reflection
    • Refactor existing array and json functions to use this so that users don't need to manually provide a base column type
    • Provide a nullable override so that the base column type can be explicitly defined

Note This is a potentially breaking change in the (unlikely) event that users were providing an array of non-primitive or unsupported types to anyFrom() or allFrom(), for example: anyFrom(arrayOf<BigInteger>()). The recommendation will be to provide the best match or custom column type to the new parameter.

@bog-walk bog-walk requested review from e5l and joc-a February 14, 2024 00:43
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

expressions looks better now, lgtm

*
* @throws IllegalStateException If no column type mapping is found and a [defaultType] is not provided.
*/
fun <T : Any> resolveColumnType(
Copy link
Member

Choose a reason for hiding this comment

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

we may want to mark it as internal API annotation

Copy link
Member Author

@bog-walk bog-walk Feb 14, 2024

Choose a reason for hiding this comment

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

@e5l I agree and I just tried but I remember now why I couldn't.
It's because it's being invoked by inline functions. So setting it to internal shows this compiler failure everywhere:

Public-API inline function cannot access non-public-API 'internal fun resolveColumnType...

Is there any option I'm not seeing? Maybe mark it with a deprecation warning or an annotation to deter public use?

- Support using ANY and ALL operators with array column or expression.
- Remove UntypedAndUnsizedArrayColumnType and replace it in AllAnyFromArrayOp
with ArrayColumnType. Remove associated unused dialect data types.
- Introduce function that resolves a column type based on type reflection. Refactor
existing array and json functions to use this, while still having a nullable override.
Fix logic in arrayParam() and arrayLiteral().

Switch tests to use type reflection and update KDocs about nullable arrays.
- Extract unrelated non-breaking changes to another PR
- Add missing Exposed column types to resolve and adjust tests
- Fix rebase inconsistency
- Update unit test to include arrayParam
- Refactor AllAnyFromExpressionOp to not perform resolve column logic
Introduce InternalApi annotation and add it to resolveColumnType().
Move other existing annotation classes to dedicated file.
@bog-walk bog-walk force-pushed the bog-walk/extend-any-all-array branch from 5eefbaf to af208a8 Compare February 20, 2024 17:18
@bog-walk bog-walk merged commit 705e106 into main Feb 20, 2024
5 checks passed
@bog-walk bog-walk deleted the bog-walk/extend-any-all-array branch February 20, 2024 17:59
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