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.

Sign up for GitHub

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

Jump to bottom

fix: test and address sanatizer after merge of unaligned and sql statements #262

Merged
merged 1 commit into from
Feb 7, 2024
Merged

fix: test and address sanatizer after merge of unaligned and sql statements #262

merged 1 commit into from
Feb 7, 2024

Conversation

JonasKellerer
Copy link
Contributor

@JonasKellerer JonasKellerer commented Jan 25, 2024 •
edited
Loading

They did not show up in the ci, but if you run the tests locally.

@JonasKellerer JonasKellerer force-pushed the fixAddressSanatizerInPreprocessingDatabase branch 3 times, most recently from 7a1539d to c5f6d54 Compare January 25, 2024 14:56
@JonasKellerer JonasKellerer requested review from Taepper and fengelniederhammer and removed request for Taepper January 25, 2024 14:56
@JonasKellerer JonasKellerer marked this pull request as ready for review January 25, 2024 14:56
@JonasKellerer JonasKellerer force-pushed the fixAddressSanatizerInPreprocessingDatabase branch from c5f6d54 to 91b4ecc Compare January 29, 2024 16:28
@JonasKellerer JonasKellerer force-pushed the fixAddressSanatizerInPreprocessingDatabase branch 2 times, most recently from cb27dd7 to f71cee1 Compare February 6, 2024 14:20
Taepper
Taepper reviewed Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@Taepper Taepper left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Wow, I like this!

Now that you managed to get it done without the static functions, we could also get rid of the circumvention that we need to pass the sequence_name and instead register a separate function for each sequence?

@JonasKellerer
Copy link
Contributor Author

Yes, we could now have a function for each of the sequences. However, I would argue to put this into a separate PR, since we need this pr so the unit tests work more reliable. But we can talk about this tomorrow.

@JonasKellerer JonasKellerer force-pushed the fixAddressSanatizerInPreprocessingDatabase branch from f71cee1 to 0d50f44 Compare February 6, 2024 16:33
@JonasKellerer JonasKellerer force-pushed the fixAddressSanatizerInPreprocessingDatabase branch from 0d50f44 to 7ac1eaf Compare February 6, 2024 17:26
fengelniederhammer
fengelniederhammer approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@fengelniederhammer fengelniederhammer left a comment

Choose a reason for hiding this comment

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

Nice!

Taepper
Taepper approved these changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@Taepper Taepper left a comment

Choose a reason for hiding this comment

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

Great! Yes, let's do that in another PR. Can you open an issue for that?

@JonasKellerer JonasKellerer merged commit c1a11c8 into main Feb 7, 2024
6 checks passed
@JonasKellerer JonasKellerer deleted the fixAddressSanatizerInPreprocessingDatabase branch February 7, 2024 10:17
@JonasKellerer
Copy link
Contributor Author

I added a new ticket for the sql functions for each of the sequences #282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Taepper Taepper Taepper approved these changes

@fengelniederhammer fengelniederhammer fengelniederhammer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JonasKellerer @Taepper @fengelniederhammer