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

[DROOLS-7540] Coercion from Interger literal to BigDecimal bind variable #337

Merged
merged 1 commit into from Sep 6, 2023

Conversation

tkobayas
Copy link
Collaborator

@tkobayas tkobayas commented Sep 6, 2023

https://issues.redhat.com/browse/DROOLS-7540

This is a fix to solve test cases in https://github.com/kiegroup/drools/pull/5498

Drools uses SimpleVariableSpaceModel to create IndexedVariableResolverFactory. What this PR does are

  • Use SimpleSTValueResolver instead of SimpleValueResolver to enable coercion
  • Enhance IndexVariableResolver to enable coercion

Comment on lines +42 to +49
if (type == Object.class || type == null) {
// IndexVariableResolver doesn't know the original declared type, so cannot deal with polymorphism
// Just accept the value
vars[indexPos] = value;
} else {
// Coerce numeric types
vars[indexPos] = handleTypeCoercion(type, value);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR makes IndexVariableResolver able to coerce numeric values.

Note that IndexVariableResolver doesn't know the original declared type, so cannot deal with polymorphism, so it just accept the value (= original behavior).

If we want to do type check, we would need to change SimpleVariableSpaceModel to store type information (or maybe creating a new class e.g. TypedVariableSpaceModel) . So we would also need to change drools side. I wonder if we want to do such a large change for this issue.

Copy link
Member

Choose a reason for hiding this comment

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

I totally agree on the fact that it doesn't make any sense to develop any change bigger than this for such a corner case issue. Good job!

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.

None yet

2 participants