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

TableHelper.inferPrimaryKey does not take into account all collumns #756

Closed
Vrolijkx opened this issue Sep 28, 2017 · 6 comments
Closed

Comments

@Vrolijkx
Copy link

If you have an abstract table with one or more implementations like below:

abstract class TimeseriesTable[Owner <: TimeseriesTable[Owner, Record], Record <: Timestamped](implicit helper: TableHelper[Owner, Record]) extends Table[Owner, Record] {
  object partitioningColl extends StringColumn with PartitionKey
  object timestamp extends DateTimeColumn with ClusteringOrder with Ascending
}

abstract class SomeTable extends TimeseriesTable[SomeTables, SomeEntity] {
  object extraPartitioning extends StringColumn with PartitionKey
  object value extends DoubleColumn
}

Then if you invoke someTable.tableKey it will only return (partitioningColl, timestamp)
instead of the expected ((partitioningColl, extraPartitioning), timestamp)

This happens because com.outworkers.phantom.macrosTableHelper.inferPrimaryKey only takes into account the TimeseriesTable and not SomeTable.

This results in an incorrect table creation + other bugs.

@alexflav23
Copy link
Member

Hi @Vrolijkx this isn't really a bug, it's a feature :) We do not openly support inheritance between tables nor do we test for it as the advantage of enabling it in terms of reducing code at the application level is fairly minimal.

If you have a different view of that, it would be interesting to hear more, but for all intents and purposes we do not expect inheritance to work at this very moment, so bugs are normal.

@Vrolijkx
Copy link
Author

@alexflav23 Would be nice if it's documented somewhere that inheritance isn't supported. And I converted back to a flat structure because was bumping also against lot's of other problems with inheritance, especially with prepared statements.

I was hoping to be able to abstract away some date partitioning logic for time series in this parent class so you could query an interval over multiple days(with data partitioned by day) and the query would be split up for you in a transparent way but inheritance doesn't seem the help me solving this problem in a reusable way.

What is the recommended way to write reusable code in Phantom?

@alexflav23
Copy link
Member

alexflav23 commented Sep 29, 2017

@Vrolijkx You don't really do a lot of re-using at the moment, as tables don't really offer that in Cassandra it's not something we've considered a lot, and you are not the first to report bugs with it.

Is it such a compelling use case for you as a library user to have that? My thinking is this, inheritance in macros will be a headache to deal with, as it's the kind of low level weird problematic thing to solve, so is it worth it?

I'm happy to be educated on where you feel this adds value, and take it into consideration internally.

@hsn10
Copy link

hsn10 commented Oct 10, 2017

If you do not want to support inheritance, you should throw compiler error from macros. Its better then silently doing unexpected thing.

@alexflav23
Copy link
Member

@hsn10 That's fair game, already working on that being done properly, you are absolutely correct this is far from ideal right now.

@alexflav23
Copy link
Member

@hsn10 @Vrolijkx We are officially going to support this for all of you requesting it, have a look at #770 to contribute to the list of requirements. We will make this happen asap.

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

No branches or pull requests

3 participants