perf(datasets): don't create connection until need

[deepyaman]

Description

Closes #366

Development notes

Checklist

• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the relevant RELEASE.md file
• Added tests to cover my changes

[noklam]

Approved with a comment on the connection refactor.

[noklam]

Suggested change

	@property
	def engine(self):
	"""The ``Engine`` object for the dataset's connection string."""
	return self.create_connection(self._connection_str)
	@property
	def engine(self):
	"""The ``Engine`` object for the dataset's connection string."""
	if self._conneciton_str not in cls.engines:
	self.create_connection(self._connection_str)
	return self.engines[self._connection_str]

The current refactoring works perfectly fine, but I feel that create_connection shouldn't return a connection and this is more natural. (the if conneciton_str not in cls.engines should be also removed).

[merelcht]

Thanks for this @deepyaman, this looks all good to me, but agree with @noklam's suggestion. I disabled the auto-merge, because I wanted to approve, but this shouldn't be merged before you check the suggestion 😄

[MatthiasRoels]

Just a quick question out of curiosity. I noticed create_connection has a cls argument, but it is used in the engine method (property), which is called with the self argument. I know that Python isn't too picky about arguments (if I remember correctly you can name self of cls whatever you want), but I would expect to have matching arguments. So either create_connection to have the self argument or engine to have cls argument...

[deepyaman]

create_connection has a cls argument because it's a class method, and it receives the class as an implicit first argument. It must receive the class (not the instance) in this method, because it modifies the .engines properly "across all instances of SQLTableDataset that need to connect to the same source", as mentioned in the docstring.

engine is a property of the instance, that uses the connection string associated with the instance, hence it must be tied to self.

[deepyaman]

@noklam @merelcht I've made the suggested change in 7170589.

Perhaps more importantly, I also realized that the connection was being created eagerly anyway in SQLQueryDataset, so I've fixed that in cb45e17.