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

create unqiue index in indexes and related children in contrib.postgres/sqlite.indexes #1642

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

omidekz
Copy link

@omidekz omidekz commented Jun 7, 2024

create a postgresql unique index and consider the nulls not distinct

Description

enhancement was started from this issue

this PR provide a manual solution for postgres driver. but i think the orm has to provide a base/shared solution.
the solution is a way to consider nulls equal in unique indexes on that drivers that consider nulls not equal.

postgres can done that by : nulls not distinct
sqlite and sqlserver too by : index expressions
on mysql i don't know yet.

@omidekz
Copy link
Author

omidekz commented Jun 8, 2024

jun 8/related changes:

  • add UniqueIndex in tortoise.indexes
  • reimplement contrib.postgres.indexes.UniqueIndex
  • implemet contribe.sqlite.indexes.UniqueIndex
    Description: todo

@omidekz omidekz force-pushed the omidekz/feature/postgres-unique-index branch 2 times, most recently from b1e4b8e to c265815 Compare June 8, 2024 20:56
@omidekz omidekz changed the title create unqiue index for postgres create unqiue index in indexes and related children in contrib.postgres/sqlite.indexes Jun 10, 2024
@abondar
Copy link
Member

abondar commented Jun 12, 2024

Why you decided to make it as separate class?
I am concerned that it complicated class hierarchy - and now, for example, it is not clear how to make unique index of non-default type for postgres, as there are now separately HashIndex for example, and Unique index, and it is not clear how to make unique HashIndex

Also CI seems to be failing due to lint, you can run make style and make lint to fix it

@omidekz
Copy link
Author

omidekz commented Jun 13, 2024

Hi i got your point. i'll remove shared unique index and go further with driver specific implementation
but i have a challenge with partialindex.
at the tortoise.indexes file line of 75 where you are using the ValueWrapper, force the k = wrapped_value. but we need k is [not] null
if i check the wrapped_value is startswith(is) and then set the operator. its mean im producing some other issues.

@omidekz omidekz force-pushed the omidekz/feature/postgres-unique-index branch from 86e0643 to 3ad96ef Compare June 13, 2024 12:24
Comment on lines +74 to +75
if self.INDEX_TYPE:
self.INDEX_TYPE = f"USING {self.INDEX_TYPE}"
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was wrong about this, I forgot that postgresql allows only default b-tree index to be unique, so it is something we don't need to support



class SqliteUniqueIndex(SqliteIndex):
INDEX_TYPE = "unique".upper()
Copy link
Member

Choose a reason for hiding this comment

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

Why not just "UNIQUE"?

fields: Optional[Tuple[str, ...]] = None,
name: Optional[str] = None,
condition: Optional[Dict[str, str]] = None,
where_expre: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

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

I am a little at lost here
Why do we need both condition and where_expre?

return str(f"{key} {op} {cond}")

def _gen_condition(self, conditions: Dict[str, str]):
conditions = " AND ".join(tuple(map(self._gen_field_cond, conditions.items())))
Copy link
Member

Choose a reason for hiding this comment

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

please use comprehensions

" AND ".join(self._gen_field_cond(k, v) for k, v in condition.items())

With that you will also be able to make better signature and typing for _gen_field_cond

self.extra = where_expre or self._gen_condition(_condition)

@classmethod
def _gen_field_cond(cls, kv: tuple):
Copy link
Member

Choose a reason for hiding this comment

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

Add return typing

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.

None yet

2 participants