Skip to content

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