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

Fix bug i hent_punkt #721

Merged
merged 2 commits into from
Dec 14, 2023
Merged

Fix bug i hent_punkt #721

merged 2 commits into from
Dec 14, 2023

Conversation

krebslw
Copy link
Collaborator

@krebslw krebslw commented Oct 26, 2023

Resolves #720

Copy link
Collaborator

@kbevers kbevers left a comment

Choose a reason for hiding this comment

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

Jeg er ret sikker på at løsningen her fungerer, men jeg lurer på om ikke det er smartere at løse problemet i hent_punkter()? Altså, sådan at hvis der er et eksakt match på identen vil det altid være det første i den returnerede liste. Med lidt snilde kan man få databasen til at gøre det. Her er et simpelt eksempel:

SELECT * FROM punktinfo pi 
WHERE pi.infotypeid=347 AND pi.tekst LIKE '%SAND%' 
ORDER BY LENGTH(pi.tekst);

Med andre ord, det korteste ident-match kommer først i listen. Det kræver at der tilføjes noget i stil med order_by(PunktInformation.tekst.length) til SQLAlchemy opslaget i hent_punkter().

Uanset om den ene eller anden tilgang bruges vil det være godt at få tilføjet en test til test-suiten, så vi er sikre på at den ny funktionalitet virker efter hensigten. Det kræver formentligt at der tilføjes et ny ident i test/sql/testdata.sql. Man kan fx tilføje et GL RDO1 til et af punkterne i testdatasættet. Kig omkring linje 300 i filen.

@krebslw
Copy link
Collaborator Author

krebslw commented Oct 26, 2023

Ja havde godt tænkt at det skulle testes.
Og tror også dit foreslag vil virke fint. Synes det tilføjer noget "implicit" logik da man så antager at det korteste navn er det rigtige. Alternativt kan man sortere således:

SELECT * FROM punktinfo pi 
WHERE pi.infotypeid=347 AND pi.tekst LIKE '%SAND%' 
ORDER BY (CASE WHEN pi.tekst='SAND' THEN 1 ELSE 2 END);

hvis det altså kan lade sig gøre sqlalchemy-style (har lige tjekket at det virker med oracle.)

Det leder også til spørgsmålet om hvilken løsning der er hurtigst, jf. issue #422, skulle vi ikke gøre det unødigt langsomt.

@kbevers
Copy link
Collaborator

kbevers commented Oct 26, 2023

Synes det tilføjer noget "implicit" logik da man så antager at det korteste navn er det rigtige.

Vi laver følgende opslag

result = (
self.session.query(Punkt)
.options(
joinedload(Punkt.geometriobjekter),
joinedload(Punkt.koordinater),
)
.join(PunktInformation)
.join(PunktInformationType)
.filter(
PunktInformationType.name.startswith("IDENT:"),
PunktInformation._registreringtil == None, # NOQA
or_(
PunktInformation.tekst == ident,
PunktInformation.tekst == f"FO {ident}",
PunktInformation.tekst == f"GL {ident}",
),
Punkt._registreringtil == None, # NOQA
)
.all()

hvor der matches på tre ting: ident, FO {ident} og GL {ident}. Det skulle meget gerne resultere i at det korteste er det eksakte match. Hvis indholdet af ident har et FO- eller GL-præfix kan det kun matche på den første. Så skulle man ende i den uheldige situation at man leder efter FO SAND, så kan man skrive det eksplicit og med sikkerhed fremfinde det.

Det leder også til spørgsmålet om hvilken løsning der er hurtigst, jf. issue #422, skulle vi ikke gøre det unødigt langsomt.

Det der er dyrt i hent_punkt og venner, er at lave mappingen mellem pythonobjekter og SQL-udtryk. Altså alle de joins der skal laves på kryds og tværs for at udfylde alle felter i Punkt. Jeg tror ikke en sortering på databasesiden koster noget nævneværdigt i den sammenhæng, men det skal times for at kunne afgøres med sikkerhed. Det er i hvert fald ikke noget jeg er synderligt bekymret for og jeg regner med en performance forbedring når vi skifter til SQLAlchemy 2.0 som tager hånd om en del af den sløvhed vi ser nu.

@kbevers
Copy link
Collaborator

kbevers commented Dec 14, 2023

@krebslw Som du kan se har jeg tilføjet en test og backportet til 1.6. Vi fik aldrig lejlighed til at lave testen sammen og nu har jeg lige brug for at dit fix kommer i spil. Brug gerne lidt tid på at læse mine tilføjelser igennem, så du har en ide om hvordan noget lignende kan gøres i fremtiden.

@krebslw krebslw deleted the bugfix branch August 15, 2024 09:16
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.

hent_punkt smider KeyError når hent_punkter returnerer mere end ét punkt
2 participants