chrthomsen / pygrametl

Official repository for pygrametl - ETL programming in Python
http://pygrametl.org
BSD 2-Clause "Simplified" License
289 stars 41 forks source link

Add missing attributes to BulkFactTable #48

Closed fromm1990 closed 1 year ago

fromm1990 commented 2 years ago

Hi, it has been discovered that the attributes all, keyrefs, and measures are missing from the BulkFactTable class. These attributes are expected to exist by the FactTablePartitioner class. The following code should raise an AttributeError exception.

import sqlite3

from pygrametl import ConnectionWrapper, parallel
from pygrametl.tables import BulkFactTable, DecoupledFactTable, FactTablePartitioner

connection = sqlite3.connect(":memory:")
cursor = connection.execute("CREATE TABLE test_table(ra INT, rb INT, ma INT, mb INT)")
connection.commit()
cursor.close()

def bulkloader(name: str, attributes, fieldsep, rowsep, nullval, filehandle):
    pass

cw = ConnectionWrapper(connection)
scw = parallel.shareconnectionwrapper(cw, userfuncs=[bulkloader])

fact = DecoupledFactTable(
    BulkFactTable(
        name="test_table",
        keyrefs=["ra", "rb"],
        measures=["ma", "mb"],
        bulkloader=scw.copy().bulkloader,
        usefilename=True
    ),
    returnvalues=False,
)

partitioner = FactTablePartitioner(parts=[fact])

print("START")
for i in range(3000):
    print(i)
    partitioner.insert({"ra": i, "rb": i % 2, "ma": 1, "mb": 2})
scw.commit()
scw.close()
print("END")
skejserjensen commented 1 year ago

@fromm1990 and @chrthomsen is this issue still relevant after #49 has been merged or can it be close?

fromm1990 commented 1 year ago

@skejserjensen I think it depends on what the issue page should reflect. As I see it, there are two options:

  1. Reflecting issues currently in the code base
  2. Reflecting issues in the released version

If 1 is preferred, the issue should be closed. If 2, then a release should be pushed out before closing this issue. What do you prefer @skejserjensen and @chrthomsen?

chrthomsen commented 1 year ago

I think an issue should be closed if a fix has been pushed to master. So I would say this issue should be closed now. Do you agree?

fromm1990 commented 1 year ago

@chrthomsen the only drawback I can think of with that approach, is that people using the version from PyPI would discover the bug and then start a new issue unaware of the code base no longer having this issue.

fromm1990 commented 1 year ago

Fixed in #49.