Skip to content

Conversation

@ghiculescu
Copy link
Contributor

Fix: #45

dump.gsub!(/^-- Name: \w+; Type: INDEX\n+/, '')
indexes.each do |table, indexes_for_table|
dump.gsub!(/^(CREATE TABLE #{table}\b(:?[^;\n]*\n)+\);\n)/) { $1 + "\n" + indexes_for_table }
dump.gsub!(/^(CREATE TABLE #{table}\b(:?[^;\n]*\n)+\);*\n(?:.*);*)/) { $1 + "\n" + indexes_for_table }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this was already complex and is getting more complex, it would be nice if this regex could live as a variable and have a corresponding Ruby class that runs a test with the different table structure DDL dump variations you mentioned.

For example one without a fillfactor or other table params, and one with that set.

If you agree, maybe we could capture that in an issue for now, if you don't have time for the unit test now, and have tested this manually and it's working for you.

@andyatkinson
Copy link
Collaborator

Seems ok to me. Any concerns @lfittl? If not, we can merge this, then add it to a new candidate release: #47

columns = []
result << source_line
elsif source_line.start_with?(");")
elsif source_line.start_with?(")")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test fails without this fix:

image

dump.gsub!(/^-- Name: \w+; Type: INDEX\n+/, '')
indexes.each do |table, indexes_for_table|
dump.gsub!(/^(CREATE TABLE #{table}\b(:?[^;\n]*\n)+\);\n)/) { $1 + "\n" + indexes_for_table }
dump.gsub!(/^(CREATE TABLE #{table}\b(:?[^;\n]*\n)+\);*\n(?:.*);*)/) { $1 + "\n\n" + indexes_for_table }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test fails without this fix:

image

@ghiculescu ghiculescu changed the title Handle tables with custom fillfactor Handle tables with custom fillfactor + add tests + add CI Sep 12, 2024
@ghiculescu
Copy link
Contributor Author

I think you will need to enable github actions on this repo for the tests to run on PRs. It might only work after this is merged.

@andyatkinson
Copy link
Collaborator

Looks really good on a quick initial read. I'll let @lfittl chime in. Nice work!

@andyatkinson
Copy link
Collaborator

@ghiculescu I don't have the "settings" tab access where I'd expect to enable GH actions. Maybe @lfittl can do that.

I did grab your branch and ran bundle exec rake test locally, and it looks good. Let's merge.

@andyatkinson andyatkinson merged commit d53cfe4 into lfittl:master Sep 15, 2024
@ghiculescu ghiculescu deleted the patch-1 branch September 18, 2024 22:56
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.

Issues with fillfactor

2 participants