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!: Add --enable-set-inference flag to import #147

Merged

Conversation

StoneDot
Copy link
Contributor

BREAKING CHANGE: import command no longer inferences set types without --enable-set-inference

Issue #, if available:

Description of changes:
This commit introduces the --enable-set-inference flag to the import command. Without this option, the import command no longer infers set types like string-set and number-set. To achieve the same behavior, you should provide the --enable-set-inference option. This resolves #139. Also, this can be considered the one solution for the #66 problem.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

BREAKING CHANGE: import command no longer inferences set types without `--enable-set-inference`
@ryota-sakamoto
Copy link
Contributor

I understand that the issue is that dynein infer type of json as ss or ns instead of list by default.

Would you add explanation of this flag on README?
Moreover, why do you decide to add flag --enable-set-inference, not --disable-set-inference for compatibility?

@christophermichaelthomasmillar

I understand that the issue is that dynein infer type of json as ss or ns instead of list by default.

Would you add explanation of this flag on README? Moreover, why do you decide to add flag --enable-set-inference, not --disable-set-inference for compatibility?

While I think its obvious that disabling set inference is the bigger change, set inference is problematic in the first-place. I personally support this choice as we would always choose to leave this option disabled and I expect that the majority of users would feel the same.

Ofc, it's no skin off my nose to need to add --disable-set-inference every time we use this tool but it's more user friendly not to have to.

@StoneDot
Copy link
Contributor Author

Moreover, why do you decide to add flag --enable-set-inference, not --disable-set-inference for compatibility?

christophermichaelthomasmillar perfectly explains my motivation for this choice. The current inference behavior for set types can be confusing for users. Based on the issues raised, it seems that users do not welcome those inferences due to their ambiguity. Additionally, we should provide a good default option for users, which is why I attempted to turn off these inferences by default.

On the other hand, including this option in the README is viable personally. However, this option may only be necessary for some users. I'm considering if this option deserves an explanation in the README or if mentioning it in the changelog would be sufficient. What are your thoughts on this?

@christophermichaelthomasmillar

On the other hand, including this option in the README is viable personally. However, this option may only be necessary for some users. I'm considering if this option deserves an explanation in the README or if mentioning it in the changelog would be sufficient. What are your thoughts on this?

I agree It makes sense to mention it in the README.
Perhaps later on down the road it might make sense to change the implementation to be smarter if someone needs this feature ( like allowing the user to supply a regex or a field name to apply set inference to).
But it no one complains?

@ryota-sakamoto
Copy link
Contributor

I understand that --enable-set-inference is more benefit rather than the other for the users.
As for README, from my opinion, we will need to add explanation about it because it makes clear for the features.

@christophermichaelthomasmillar

This PR seems to be stalled. For my part we are about to need to use this program again for some migrations and it would be great to have an official build of this tool. For practical purposes until that happens It means that migration work needs to be done by me. Any chance we can have this merged as is and defer any further improvements to the next release?

@StoneDot
Copy link
Contributor Author

StoneDot commented Aug 3, 2023

I'm sorry for the delay of my working. I had a little difficulty for my health. @ryota-sakamoto Based on feedbacks, I have added description for --enable-set-inference. Could you please look at this?

@StoneDot StoneDot force-pushed the add-flag-to-disable-type-inference branch from 3f63e55 to fd97caf Compare August 4, 2023 01:55
@ryota-sakamoto ryota-sakamoto merged commit 9e21655 into awslabs:main Aug 4, 2023
1 check passed
@StoneDot StoneDot deleted the add-flag-to-disable-type-inference branch October 17, 2023 07:22
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.

Unreasonable type guessing for SS and NS causes Validation error when importing backup
3 participants