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

Shared: Generalize the number of columns in a generated MaD row #18612

Merged
merged 3 commits into from
Jan 29, 2025

Conversation

paldepind
Copy link
Contributor

@paldepind paldepind commented Jan 28, 2025

Currently the partialModel predicate in ModelGeneratorInputSig requires exactly 5 columns to specify each target for a model. In Rust we only need 2 columns to identify a function. This PR generalizes the partialModel to a partialModelRow where any number of columns is supported.

@paldepind paldepind force-pushed the shared-model-generation-row branch from 598d4ec to 13e0829 Compare January 28, 2025 14:36
@paldepind paldepind marked this pull request as ready for review January 28, 2025 15:19
@paldepind paldepind requested review from a team as code owners January 28, 2025 15:19
@paldepind paldepind assigned paldepind and unassigned paldepind Jan 28, 2025
@paldepind paldepind added the no-change-note-required This PR does not need a change note label Jan 28, 2025
aschackmull
aschackmull previously approved these changes Jan 29, 2025
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Very nice approach!

or
i = 4 and ExternalFlow::partialModel(api, _, _, _, _, result) // parameters
or
i = 5 and result = "" and exists(api) // ext
Copy link
Contributor

@michaelnebel michaelnebel Jan 29, 2025

Choose a reason for hiding this comment

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

So replacing the conjunction with disjuncts (and index) and a subsequent concat works because the values in each column is a function of the api (just an observation and not anything that requires action).

+ parameters + ";" //
+ /* ext + */ ";" //
)
result = concat(int i | | Lang::partialModelRow(api, i), ";" order by i) + ";"
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use strictconcat instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, there should always be at least one column.

+ name + ";" //
+ parameters + ";" //
)
result = concat(int i | | Lang::partialNeutralModelRow(api, i), ";" order by i) + ";"
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above.

Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@paldepind paldepind merged commit e141b4e into github:main Jan 29, 2025
33 checks passed
@paldepind paldepind deleted the shared-model-generation-row branch January 29, 2025 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants