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

make sql statements customizable via options #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pablote
Copy link

@pablote pablote commented Apr 10, 2020

Hi! great lib!

Not sure how you'll feel about this changes, since you didn't asked for them. But I needed this for my use case. Mainly, I needed to be able to override the CREATE stmt for the migrations table, since my DB is not exactly SQL compliant. Also, I tried to reduce the use of string interpolation for SQL.

@pablote pablote force-pushed the feature/custom-sql branch 2 times, most recently from 324d121 to 46fe247 Compare April 11, 2020 00:15
@lopezator
Copy link
Owner

Hola, no he tenido tiempo de revisar el código... Pero tengo curiosidad, que base de datos utilizas exactamente?

No está soportada por database/sql?

Gracias por usar migrator y por compartir tu código.

Mañana espero poder darle una pasada.

@pablote
Copy link
Author

pablote commented Apr 13, 2020

Hola. La base de datos es ClickHouse. Si la soporta database/sql, pero el CREATE para la tabla de migrations asi como estaba no funcionaba, por eso necesitaba una forma de poder overridearlo.

@lopezator
Copy link
Owner

lopezator commented Apr 14, 2020

Te importaría enviarme los statements que necesitarías para crear la database e insertar la versión? Toda información extra ayuda.

Me gustaría darle una pensada extra, igual se nos ocurre otra manera con menos impacto en el código, un flag o algo así...

@pablote
Copy link
Author

pablote commented Apr 14, 2020

Ahi va el codigo completo que estoy usando, se ve el CREATE ahi:

const createSqlFormat = `
CREATE TABLE IF NOT EXISTS %s
(
	id Int64,
	version String
) ENGINE = MergeTree()
  ORDER BY (id);
`

func RunMigrations(db *sql.DB) error {
	m, err := migrator.New(migrator.Migrations(migrations.New()...), migrator.WithCreateSqlFormat(createSqlFormat))

	if err != nil {
		return err
	}

	if err := m.Migrate(db); err != nil {
		return err
	}

	return nil
}

En Clickhouse no hay indices basicamente, sino que las tablas tienen un order preestablecido. Similar a los clustered indexes. Es un producto mas del palo de analytics que de las bases de datos transaccionales, y el SQL que implementa es muy parecido al standard pero no 100% compatible.

Me parecio buena la idea de poder overridear de afuera los statements para que la lib no tenga que implementar cada posible dialecto de SQL, que hay unos cuantos.

@lopezator
Copy link
Owner

Entendido @pablote !!

Me gustaría pensarlo un poco mejor para ver si puede encajar y como e intentar que cubriese cualquier uso con el mínimo impacto en el codebase.

Si mientras tanto puedes tirar de un fork sería ideal.

Acaba de entrar un issue que podría estar relacionado, si te parece lo enlazo allí.

@lopezator lopezator added the needs investigation The issue or pull request need to be investigated further label Apr 15, 2020
@pablote
Copy link
Author

pablote commented Apr 15, 2020

Sure, think about it, I'm fine for moment just using my fork.

I've looked at that other issue, and what it suggests is much more ambitious than this, not only being independent from the SQL statements themselves, but from database/sql completely.

I've actually worked with umzug before, and it's not that much different from this lib. Imagine migrations defined the same as they are now, but instead of receiving a *sql.Tx, being able receive anything. So you can use migrations for anything that you want. Umzug is usually paired up with sequelize which is an ORM, so what you usually get is a sequelize instance, the ORM handles all database/sql differences.

It's a very flexible design, but maybe too flexible, I don't think depending on database/sql is a problem if the scope is providing a database migration lib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs investigation The issue or pull request need to be investigated further
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants